From: Sean Anderson <hidden> Date: 2021-05-11 19:13:03
This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
a "soft" block, so it has many parameters which would not be
configurable in most hardware. This binding is usually automatically
generated by Xilinx's tools, so the names and values of some properties
must be kept as they are. Replacement properties have been provided for
new device trees.
Because we need to init timer devices so early in boot, the easiest way
to configure things is to use a device tree property. For the moment
this is 'xlnx,pwm', but this could be extended/renamed/etc. in the
future if these is a need for a generic property.
Signed-off-by: Sean Anderson <redacted>
---
How should the clocking situation be documented? For the moment I have
just left clock as optional, but should clock-frequency be documented?
Changes in v3:
- Mark all boolean-as-int properties as deprecated
- Add xlnx,pwm and xlnx,gen?-active-low properties.
- Make newer replacement properties mutually-exclusive with what they
replace
- Add an example with non-deprecated properties only.
Changes in v2:
- Use 32-bit addresses for example binding
.../bindings/pwm/xlnx,axi-timer.yaml | 142 ++++++++++++++++++
1 file changed, 142 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
@@ -0,0 +1,142 @@+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause+%YAML1.2+---+$id:http://devicetree.org/schemas/pwm/xlnx,axi-timer.yaml#+$schema:http://devicetree.org/meta-schemas/core.yaml#++title:Xilinx LogiCORE IP AXI Timer Device Tree Binding++maintainers:+-Sean Anderson <sean.anderson@seco.com>++properties:+compatible:+oneOf:+-items:+-const:xlnx,axi-timer-2.0+-const:xlnx,xps-timer-1.00.a+-const:xlnx,xps-timer-1.00.a++clocks:+maxItems:1++clock-names:+const:s_axi_aclk++reg:+maxItems:1++xlnx,count-width:+$ref:/schemas/types.yaml#/definitions/uint32+minimum:8+maximum:32+description:+The width of the counter(s), in bits.++xlnx,gen0-assert:+$ref:/schemas/types.yaml#/definitions/uint32+enum:[0,1]+default:1+deprecated:true+description:+The polarity of the generateout0 signal. 0 for active-low, 1 for active-high.++xlnx,gen0-active-low:+$ref:/schemas/types.yaml#/definitions/flag+description:+The generate0 signal is active-low.++xlnx,gen1-assert:+$ref:/schemas/types.yaml#/definitions/uint32+enum:[0,1]+default:1+deprecated:true+description:+The polarity of the generateout1 signal. 0 for active-low, 1 for active-high.++xlnx,gen1-active-low:+$ref:/schemas/types.yaml#/definitions/flag+description:+The generate1 signal is active-low.++xlnx,one-timer-only:+$ref:/schemas/types.yaml#/definitions/uint32+enum:[0,1]+deprecated:true+description:+Whether only one timer is present in this block.++xlnx,single-timer:+$ref:/schemas/types.yaml#/definitions/flag+description:+Only one timer is present in this block.++xlnx,pwm:+$ref:/schemas/types.yaml#/definitions/flag+description:+This timer should be configured as a PWM.++required:+-compatible+-reg+-xlnx,count-width++allOf:+-if:+required:+-clocks+then:+required:+-clock-names++-if:+required:+-xlnx,gen0-active-low+then:+not:+required:+-xlnx,gen0-assert++-if:+required:+-xlnx,gen0-active-low+then:+not:+required:+-xlnx,gen0-assert++-if:+required:+-xlnx,one-timer-only+then:+not:+required:+-xlnx,single-timer++additionalProperties:true++examples:+-|+axi_timer_0:timer@800e0000 {+clock-names = "s_axi_aclk";+clocks = <&zynqmp_clk 71>;+compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";+reg = <0x800e0000 0x10000>;+xlnx,count-width = <0x20>;+xlnx,gen0-assert = <0x1>;+xlnx,gen1-assert = <0x1>;+xlnx,one-timer-only = <0x0>;+xlnx,trig0-assert = <0x1>;+xlnx,trig1-assert = <0x1>;+};++-|+axi_timer_0:timer@800e0000 {+clock-names = "s_axi_aclk";+clocks = <&zynqmp_clk 71>;+compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";+reg = <0x800e0000 0x10000>;+xlnx,count-width = <0x20>;+xlnx,gen0-active-low;+xlnx,single-timer;+};
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Sean Anderson <hidden> Date: 2021-05-11 19:13:07
This adds generic clocksource and clockevent support for Xilinx LogiCORE IP
AXI soft timers commonly found on Xilinx FPGAs. This timer is also the
primary timer for Microblaze processors. This commit also adds support for
configuring this timer as a PWM (though this could be split off if
necessary). This whole driver lives in clocksource because it is primarily
clocksource stuff now (even though it started out as a PWM driver). I think
teasing apart the driver would not be worth it since they share so many
functions.
This driver configures timer 0 (which is always present) as a clocksource,
and timer 1 (which might be missing) as a clockevent. I don't know if this
is the correct priority for these timers, or whether we should be using a
more dynamic allocation scheme.
At the moment clock control is very basic: we just enable the clock during
probe and pin the frequency. In the future, someone could add support for
disabling the clock when not in use. Cascade mode is also unsupported.
This driver was written with reference to Xilinx DS764 for v1.03.a [1].
[1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
Signed-off-by: Sean Anderson <redacted>
---
Please let me know if I should organize this differently or if it should
be broken up.
Changes in v3:
- Add clockevent and clocksource support
- Rewrite probe to only use a device_node, since timers may need to be
initialized before we have proper devices. This does bloat the code a bit
since we can no longer rely on helpers such as dev_err_probe. We also
cannot rely on device resources being free'd on failure, so we must free
them manually.
- We now access registers through xilinx_timer_(read|write). This allows us
to deal with endianness issues, as originally seen in the microblaze
driver. CAVEAT EMPTOR: I have not tested this on big-endian!
- Remove old microblaze driver
Changes in v2:
- Don't compile this module by default for arm64
- Add dependencies on COMMON_CLK and HAS_IOMEM
- Add comment explaining why we depend on !MICROBLAZE
- Add comment describing device
- Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
- Use NSEC_TO_SEC instead of defining our own
- Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by Uwe
- Cast dividends to u64 to avoid overflow
- Check for over- and underflow when calculating TLR
- Set xilinx_pwm_ops.owner
- Don't set pwmchip.base to -1
- Check range of xlnx,count-width
- Ensure the clock is always running when the pwm is registered
- Remove debugfs file :l
- Report errors with dev_error_probe
arch/microblaze/kernel/Makefile | 2 +-
arch/microblaze/kernel/timer.c | 326 ---------------
drivers/clocksource/Kconfig | 15 +
drivers/clocksource/Makefile | 1 +
drivers/clocksource/timer-xilinx.c | 650 +++++++++++++++++++++++++++++
5 files changed, 667 insertions(+), 327 deletions(-)
delete mode 100644 arch/microblaze/kernel/timer.c
create mode 100644 drivers/clocksource/timer-xilinx.c
@@ -0,0 +1,650 @@+// SPDX-License-Identifier: GPL-2.0++/*+*Copyright(C)2021SeanAnderson<sean.anderson@seco.com>+*+*ForXilinxLogiCOREIPAXITimerdocumentation,refertoDS764:+*https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf+*+*Hardwarelimitations:+*-Whenincascademodewecannotreadthefull64-bitcounterinonego+*-Whenchangingbothdutycycleandperiod,wemayendupwithonecycle+*withtheolddutycycleandthenewperiod.+*-Cannotproduce100%dutycycle.+*-Onlyproduces"normal"output.+*/+#include<linux/bitfield.h>+#include<linux/clk.h>+#include<linux/clk-provider.h>+#include<linux/clockchips.h>+#include<linux/clocksource.h>+#include<linux/device.h>+#include<linux/interrupt.h>+#include<linux/module.h>+#include<linux/of_irq.h>+#include<linux/of_address.h>+#include<linux/platform_device.h>+#include<linux/pwm.h>+#include<linux/sched_clock.h>+#include<asm/io.h>+#if IS_ENABLED(CONFIG_MICROBLAZE)+#include<asm/cpuinfo.h>+#endif++/* A replacement for dev_err_probe, since we don't always have a device */+#define xilinx_timer_err(np, err, fmt, ...) ({ \+pr_err("%pOF: error %d: "fmt,(np),(int)(err),##__VA_ARGS__);\+err;\+})++#define TCSR0 0x00+#define TLR0 0x04+#define TCR0 0x08+#define TCSR1 0x10+#define TLR1 0x14+#define TCR1 0x18++#define TCSR_MDT BIT(0)+#define TCSR_UDT BIT(1)+#define TCSR_GENT BIT(2)+#define TCSR_CAPT BIT(3)+#define TCSR_ARHT BIT(4)+#define TCSR_LOAD BIT(5)+#define TCSR_ENIT BIT(6)+#define TCSR_ENT BIT(7)+#define TCSR_TINT BIT(8)+#define TCSR_PWMA BIT(9)+#define TCSR_ENALL BIT(10)+#define TCSR_CASC BIT(11)++/*+*TheideahereistocapturewhetherthePWMisactuallyrunning(e.g.+*becauseweorthebootloadersetitup)andweneedtobecarefultoensure+*wedon'tcauseaglitch.Accordingtothedevicedatasheet,toenablethe+*PWMweneedto+*+*-Setbothtimerstogeneratemode(MDT=1)+*-SetbothtimerstoPWMmode(PWMA=1)+*-Enablethegenerateoutsignals(GENT=1)+*+*Inaddition,+*+*-Thetimermustberunning(ENT=1)+*-Thetimermustauto-reloadTLRintoTCR(ARHT=1)+*-WemustnotbeintheprocessofloadingTLRintoTCR(LOAD=0)+*-Cascademodemustbedisabled(CASC=0)+*+*Ifanyofthesedifferfromusual,thenthePWMiseitherdisabled,oris+*runninginamodethatthisdriverdoesnotsupport.+*/+#define TCSR_PWM_SET (TCSR_GENT | TCSR_ARHT | TCSR_ENT | TCSR_PWMA)+#define TCSR_PWM_CLEAR (TCSR_MDT | TCSR_LOAD)+#define TCSR_PWM_MASK (TCSR_PWM_SET | TCSR_PWM_CLEAR)++/**+*structxilinx_timer_priv-PrivatedataforXilinxAXItimerdriver+*@cs:Clocksourcedevice+*@ce:Clockeventdevice+*@pwm:PWMcontrollerchip+*@clk:Parentclock+*@regs:Baseaddressofthisdevice+*@width:Widthofthecounters,inbits+*@XILINX_TIMER_ONE:Wehaveonlyonetimer.+*@XILINX_TIMER_PWM:ConfiguredasaPWM.+*@XILINX_TIMER_CLK:Weweremissingadevicetreeclockandcreatedourown+*@flags:Flagsforwhattypeofdeviceweare+*/+structxilinx_timer_priv{+union{+struct{+structclocksourcecs;+structclock_event_devicece;+};+structpwm_chippwm;+};+structclk*clk;+void__iomem*regs;+u32(*read)(constvolatilevoid__iomem*addr);+void(*write)(u32value,volatilevoid__iomem*addr);+unsignedintwidth;+enum{+XILINX_TIMER_ONE=BIT(0),+XILINX_TIMER_PWM=BIT(1),+XILINX_TIMER_CLK=BIT(2),+}flags;+};++staticinlinestructxilinx_timer_priv+*xilinx_pwm_chip_to_priv(structpwm_chip*chip)+{+returncontainer_of(chip,structxilinx_timer_priv,pwm);+}++staticinlinestructxilinx_timer_priv+*xilinx_clocksource_to_priv(structclocksource*cs)+{+returncontainer_of(cs,structxilinx_timer_priv,cs);+}++staticinlinestructxilinx_timer_priv+*xilinx_clockevent_to_priv(structclock_event_device*ce)+{+returncontainer_of(ce,structxilinx_timer_priv,ce);+}++staticu32xilinx_ioread32be(constvolatilevoid__iomem*addr)+{+returnioread32be(addr);+}++staticvoidxilinx_iowrite32be(u32value,volatilevoid__iomem*addr)+{+iowrite32be(value,addr);+}++staticinlineu32xilinx_timer_read(structxilinx_timer_priv*priv,+intoffset)+{+returnpriv->read(priv->regs+offset);+}++staticinlinevoidxilinx_timer_write(structxilinx_timer_priv*priv,+u32value,intoffset)+{+priv->write(value,priv->regs+offset);+}++staticinlineu64xilinx_timer_max(structxilinx_timer_priv*priv)+{+returnBIT_ULL(priv->width)-1;+}++staticintxilinx_timer_tlr_cycles(structxilinx_timer_priv*priv,u32*tlr,+u32tcsr,u64cycles)+{+u64max_count=xilinx_timer_max(priv);++if(cycles<2||cycles>max_count+2)+return-ERANGE;++if(tcsr&TCSR_UDT)+*tlr=cycles-2;+else+*tlr=max_count-cycles+2;++return0;+}++staticboolxilinx_timer_pwm_enabled(u32tcsr0,u32tcsr1)+{+return((TCSR_PWM_MASK|TCSR_CASC)&tcsr0)==TCSR_PWM_SET&&+(TCSR_PWM_MASK&tcsr1)==TCSR_PWM_SET;+}++staticintxilinx_timer_tlr_period(structxilinx_timer_priv*priv,u32*tlr,+u32tcsr,unsignedintperiod)+{+u64cycles=DIV_ROUND_DOWN_ULL((u64)period*clk_get_rate(priv->clk),+NSEC_PER_SEC);++returnxilinx_timer_tlr_cycles(priv,tlr,tcsr,cycles);+}++staticunsignedintxilinx_timer_get_period(structxilinx_timer_priv*priv,+u32tlr,u32tcsr)+{+u64cycles;++if(tcsr&TCSR_UDT)+cycles=tlr+2;+else+cycles=xilinx_timer_max(priv)-tlr+2;++returnDIV_ROUND_UP_ULL(cycles*NSEC_PER_SEC,+clk_get_rate(priv->clk));+}++staticintxilinx_pwm_apply(structpwm_chip*chip,structpwm_device*unused,+conststructpwm_state*state)+{+intret;+structxilinx_timer_priv*priv=xilinx_pwm_chip_to_priv(chip);+u32tlr0,tlr1;+u32tcsr0=xilinx_timer_read(priv,TCSR0);+u32tcsr1=xilinx_timer_read(priv,TCSR1);+boolenabled=xilinx_timer_pwm_enabled(tcsr0,tcsr1);++if(state->polarity!=PWM_POLARITY_NORMAL)+return-EINVAL;++ret=xilinx_timer_tlr_period(priv,&tlr0,tcsr0,state->period);+if(ret)+returnret;++ret=xilinx_timer_tlr_period(priv,&tlr1,tcsr1,state->duty_cycle);+if(ret)+returnret;++xilinx_timer_write(priv,tlr0,TLR0);+xilinx_timer_write(priv,tlr1,TLR1);++if(state->enabled){+/* Only touch the TCSRs if we aren't already running */+if(!enabled){+/* Load TLR into TCR */+xilinx_timer_write(priv,tcsr0|TCSR_LOAD,TCSR0);+xilinx_timer_write(priv,tcsr1|TCSR_LOAD,TCSR1);+/* Enable timers all at once with ENALL */+tcsr0=(TCSR_PWM_SET&~TCSR_ENT)|(tcsr0&TCSR_UDT);+tcsr1=TCSR_PWM_SET|TCSR_ENALL|(tcsr1&TCSR_UDT);+xilinx_timer_write(priv,tcsr0,TCSR0);+xilinx_timer_write(priv,tcsr1,TCSR1);+}+}else{+xilinx_timer_write(priv,0,TCSR0);+xilinx_timer_write(priv,0,TCSR1);+}++return0;+}++staticvoidxilinx_pwm_get_state(structpwm_chip*chip,+structpwm_device*unused,+structpwm_state*state)+{+structxilinx_timer_priv*priv=xilinx_pwm_chip_to_priv(chip);+u32tlr0=xilinx_timer_read(priv,TLR0);+u32tlr1=xilinx_timer_read(priv,TLR1);+u32tcsr0=xilinx_timer_read(priv,TCSR0);+u32tcsr1=xilinx_timer_read(priv,TCSR1);++state->period=xilinx_timer_get_period(priv,tlr0,tcsr0);+state->duty_cycle=xilinx_timer_get_period(priv,tlr1,tcsr1);+state->enabled=xilinx_timer_pwm_enabled(tcsr0,tcsr1);+state->polarity=PWM_POLARITY_NORMAL;+}++staticconststructpwm_opsxilinx_pwm_ops={+.apply=xilinx_pwm_apply,+.get_state=xilinx_pwm_get_state,+.owner=THIS_MODULE,+};++staticintxilinx_pwm_init(structdevice*dev,+structxilinx_timer_priv*priv)+{+intret;++if(!dev)+return-EPROBE_DEFER;++priv->pwm.dev=dev;+priv->pwm.ops=&xilinx_pwm_ops;+priv->pwm.npwm=1;+ret=pwmchip_add(&priv->pwm);+if(ret)+xilinx_timer_err(dev->of_node,ret,+"could not register pwm chip\n");+returnret;+}++staticirqreturn_txilinx_timer_handler(intirq,void*dev)+{+structxilinx_timer_priv*priv=dev;+u32tcsr1=xilinx_timer_read(priv,TCSR1);++/* Acknowledge interrupt */+xilinx_timer_write(priv,tcsr1|TCSR_TINT,TCSR1);+priv->ce.event_handler(&priv->ce);+returnIRQ_HANDLED;+}++staticintxilinx_clockevent_next_event(unsignedlongevt,+structclock_event_device*ce)+{+structxilinx_timer_priv*priv=xilinx_clockevent_to_priv(ce);++xilinx_timer_write(priv,evt,TLR1);+xilinx_timer_write(priv,TCSR_LOAD,TCSR1);+xilinx_timer_write(priv,TCSR_ENIT|TCSR_ENT,TCSR1);+return0;+}++staticintxilinx_clockevent_state_periodic(structclock_event_device*ce)+{+intret;+u32tlr1;+structxilinx_timer_priv*priv=xilinx_clockevent_to_priv(ce);++ret=xilinx_timer_tlr_cycles(priv,&tlr1,0,+clk_get_rate(priv->clk)/HZ);+if(ret)+returnret;++xilinx_timer_write(priv,tlr1,TLR1);+xilinx_timer_write(priv,TCSR_LOAD,TCSR1);+xilinx_timer_write(priv,TCSR_ARHT|TCSR_ENIT|TCSR_ENT,TCSR1);+return0;+}++staticintxilinx_clockevent_shutdown(structclock_event_device*ce)+{+xilinx_timer_write(xilinx_clockevent_to_priv(ce),0,TCSR1);+return0;+}++staticconststructclock_event_devicexilinx_clockevent_base={+.name="xilinx_clockevent",+.features=CLOCK_EVT_FEAT_PERIODIC|CLOCK_EVT_FEAT_ONESHOT,+.set_next_event=xilinx_clockevent_next_event,+.set_state_periodic=xilinx_clockevent_state_periodic,+.set_state_shutdown=xilinx_clockevent_shutdown,+.rating=300,+.cpumask=cpu_possible_mask,+.owner=THIS_MODULE,+};++staticintxilinx_clockevent_init(structdevice_node*np,+structxilinx_timer_priv*priv)+{+intret=of_irq_get(np,0);++if(ret<0)+returnxilinx_timer_err(np,ret,"could not get irq\n");++ret=request_irq(ret,xilinx_timer_handler,IRQF_TIMER,+np->full_name,priv);+if(ret)+returnxilinx_timer_err(np,ret,"could not request irq\n");++memcpy(&priv->ce,&xilinx_clockevent_base,sizeof(priv->ce));+clockevents_config_and_register(&priv->ce,+clk_get_rate(priv->clk),2,+min_t(u64,+xilinx_timer_max(priv)+2,+ULONG_MAX));+return0;+}++staticu64xilinx_clocksource_read(structclocksource*cs)+{+returnxilinx_timer_read(xilinx_clocksource_to_priv(cs),TCR0);+}++staticconststructclocksourcexilinx_clocksource_base={+.read=xilinx_clocksource_read,+.name="xilinx_clocksource",+.rating=300,+.flags=CLOCK_SOURCE_IS_CONTINUOUS,+.owner=THIS_MODULE,+};++staticintxilinx_clocksource_init(structxilinx_timer_priv*priv)+{+xilinx_timer_write(priv,0,TLR0);+/* Load TLR and clear any interrupts */+xilinx_timer_write(priv,TCSR_LOAD|TCSR_TINT,TCSR0);+/* Start the timer counting up with auto-reload */+xilinx_timer_write(priv,TCSR_ARHT|TCSR_ENT,TCSR0);++memcpy(&priv->cs,&xilinx_clocksource_base,sizeof(priv->cs));+priv->cs.mask=xilinx_timer_max(priv);+returnclocksource_register_hz(&priv->cs,clk_get_rate(priv->clk));+}++staticstructclk*xilinx_timer_clock_init(structdevice_node*np,+structxilinx_timer_priv*priv)+{+intret;+u32freq;+structclk_hw*hw;+structclk*clk=of_clk_get_by_name(np,"s_axi_aclk");++if(!IS_ERR(clk)||PTR_ERR(clk)==-EPROBE_DEFER)+returnclk;++pr_warn("%pOF: missing s_axi_aclk, falling back to clock-frequency\n",+np);+ret=of_property_read_u32(np,"clock-frequency",&freq);+if(ret){+#if IS_ENABLED(CONFIG_MICROBLAZE)+pr_warn("%pOF: missing clock-frequency, falling back to /cpus/timebase-frequency\n",+np);+freq=cpuinfo.cpu_clock_freq;+#else+returnERR_PTR(ret);+#endif+}++priv->flags|=XILINX_TIMER_CLK;+hw=__clk_hw_register_fixed_rate(NULL,np,"s_axi_aclk",NULL,NULL,+NULL,0,freq,0,0);+if(IS_ERR(hw))+returnERR_CAST(hw);+returnhw->clk;+}++staticstructxilinx_timer_priv*xilinx_timer_init(structdevice*dev,+structdevice_node*np)+{+boolpwm;+inti,ret;+structxilinx_timer_priv*priv;+u32one_timer,tcsr0;++priv=kzalloc(sizeof(*priv),GFP_KERNEL);+if(!priv)+returnERR_PTR(-ENOMEM);++priv->regs=of_iomap(np,0);+if(!priv->regs){+ret=-ENXIO;+gotoerr_priv;+}elseif(IS_ERR(priv->regs)){+ret=PTR_ERR(priv->regs);+gotoerr_priv;+}++priv->read=ioread32;+priv->write=iowrite32;+/*+*Wearen'tusingtheinterruptsyet,souseENITtodetectendianness+*/+tcsr0=xilinx_timer_read(priv,TCSR0);+if(swab32(tcsr0)&TCSR_ENIT){+ret=xilinx_timer_err(np,-EOPNOTSUPP,+"cannot determine endianness\n");+gotoerr_priv;+}++xilinx_timer_write(priv,tcsr0|TCSR_ENIT,TCSR0);+if(!(xilinx_timer_read(priv,TCSR0)&TCSR_ENIT)){+priv->read=xilinx_ioread32be;+priv->write=xilinx_iowrite32be;+}++/*+*Forbackwardscompatibility,allowxlnx,one-timer-only=<bool>;+*However,thepreferredwayistousethexlnx,single-timerflag.+*/+one_timer=of_property_read_bool(np,"xlnx,single-timer");+if(!one_timer){+ret=of_property_read_u32(np,"xlnx,one-timer-only",&one_timer);+if(ret){+ret=xilinx_timer_err(np,ret,"xlnx,one-timer-only");+gotoerr_priv;+}+}++pwm=of_property_read_bool(np,"xlnx,pwm");+if(one_timer&&pwm){+ret=xilinx_timer_err(np,-EINVAL,+"pwm mode not possible with one timer\n");+gotoerr_priv;+}++priv->flags=FIELD_PREP(XILINX_TIMER_ONE,one_timer)|+FIELD_PREP(XILINX_TIMER_PWM,pwm);++for(i=0;pwm&&i<2;i++){+charint_fmt[]="xlnx,gen%u-assert";+charbool_fmt[]="xlnx,gen%u-active-low";+charbuf[max(sizeof(int_fmt),sizeof(bool_fmt))];+u32gen;++/*+*Allowxlnx,gen?-assert=<bool>;forbackwards+*compatibility.However,thepreferredwayistousethe+*xlnx,gen?-active-lowflag.+*/+snprintf(buf,sizeof(buf),bool_fmt,i);+gen=!of_property_read_bool(np,buf);+if(gen){+snprintf(buf,sizeof(buf),int_fmt,i);+ret=of_property_read_u32(np,buf,&gen);+if(ret&&ret!=-EINVAL){+xilinx_timer_err(np,ret,"%s\n",buf);+gotoerr_priv;+}+}++if(!gen){+ret=xilinx_timer_err(np,-EINVAL,+"generateout%u must be active high\n",+i);+gotoerr_priv;+}+}++ret=of_property_read_u32(np,"xlnx,count-width",&priv->width);+if(ret){+xilinx_timer_err(np,ret,"xlnx,count-width\n");+gotoerr_priv;+}elseif(priv->width<8||priv->width>32){+ret=xilinx_timer_err(np,-EINVAL,"invalid counter width\n");+gotoerr_priv;+}++priv->clk=xilinx_timer_clock_init(np,priv);+if(IS_ERR(priv->clk)){+ret=xilinx_timer_err(np,PTR_ERR(priv->clk),"clock\n");+gotoerr_priv;+}++ret=clk_prepare_enable(priv->clk);+if(ret){+xilinx_timer_err(np,ret,"clock enable failed\n");+gotoerr_clk;+}+clk_rate_exclusive_get(priv->clk);++if(pwm){+ret=xilinx_pwm_init(dev,priv);+}else{+ret=xilinx_clocksource_init(priv);+if(!ret&&!one_timer){+ret=xilinx_clockevent_init(np,priv);+if(ret)+priv->flags|=XILINX_TIMER_ONE;+}+}++if(!ret)+returnpriv;++clk_rate_exclusive_put(priv->clk);+clk_disable_unprepare(priv->clk);+err_clk:+if(priv->flags&XILINX_TIMER_CLK)+clk_unregister_fixed_rate(priv->clk);+else+clk_put(priv->clk);+err_priv:+kfree(priv);+returnERR_PTR(ret);+}++staticintxilinx_timer_probe(structplatform_device*pdev)+{+structxilinx_timer_priv*priv=+xilinx_timer_init(&pdev->dev,pdev->dev.of_node);++if(IS_ERR(priv))+returnPTR_ERR(priv);++platform_set_drvdata(pdev,priv);+return0;+}++staticintxilinx_timer_remove(structplatform_device*pdev)+{+structxilinx_timer_priv*priv=platform_get_drvdata(pdev);++if(IS_ENABLED(CONFIG_XILINX_PWM)&&priv->flags&XILINX_TIMER_PWM){+pwmchip_remove(&priv->pwm);+}else{+if(!(priv->flags&XILINX_TIMER_ONE)){+intcpu;++for_each_cpu(cpu,priv->ce.cpumask)+clockevents_unbind_device(&priv->ce,cpu);+}+clocksource_unregister(&priv->cs);+}++clk_rate_exclusive_put(priv->clk);+clk_disable_unprepare(priv->clk);+if(priv->flags&XILINX_TIMER_CLK)+clk_unregister_fixed_rate(priv->clk);+else+clk_put(priv->clk);+return0;+}++staticconststructof_device_idxilinx_timer_of_match[]={+{.compatible="xlnx,xps-timer-1.00.a",},+{.compatible="xlnx,axi-timer-2.0"},+{},+};+MODULE_DEVICE_TABLE(of,xilinx_timer_of_match);++staticstructplatform_driverxilinx_timer_driver={+.probe=xilinx_timer_probe,+.remove=xilinx_timer_remove,+.driver={+.name="xilinx-timer",+.of_match_table=of_match_ptr(xilinx_timer_of_match),+},+};+module_platform_driver(xilinx_timer_driver);++staticstructxilinx_timer_priv*xilinx_sched=(void*)-EAGAIN;++staticu64xilinx_sched_read(void)+{+returnxilinx_timer_read(xilinx_sched,TCSR0);+}++staticint__initxilinx_timer_register(structdevice_node*np)+{+structxilinx_timer_priv*priv;++if(xilinx_sched!=ERR_PTR(-EAGAIN))+return-EPROBE_DEFER;++priv=xilinx_timer_init(NULL,np);+if(IS_ERR(priv))+returnPTR_ERR(priv);+of_node_set_flag(np,OF_POPULATED);++xilinx_sched=priv;+sched_clock_register(xilinx_sched_read,priv->width,+clk_get_rate(priv->clk));+return0;+}++TIMER_OF_DECLARE(xilinx_xps_timer,"xlnx,xps-timer-1.00.a",xilinx_timer_register);+TIMER_OF_DECLARE(xilinx_axi_timer,"xlnx,axi-timer-2.0",xilinx_timer_register);++MODULE_ALIAS("platform:xilinx-timer");+MODULE_DESCRIPTION("Xilinx LogiCORE IP AXI Timer driver");+MODULE_LICENSE("GPL v2");
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Sean Anderson <hidden> Date: 2021-05-11 19:19:17
On 5/11/21 3:12 PM, Sean Anderson wrote:
quoted hunk
This adds generic clocksource and clockevent support for Xilinx LogiCORE IP
AXI soft timers commonly found on Xilinx FPGAs. This timer is also the
primary timer for Microblaze processors. This commit also adds support for
configuring this timer as a PWM (though this could be split off if
necessary). This whole driver lives in clocksource because it is primarily
clocksource stuff now (even though it started out as a PWM driver). I think
teasing apart the driver would not be worth it since they share so many
functions.
This driver configures timer 0 (which is always present) as a clocksource,
and timer 1 (which might be missing) as a clockevent. I don't know if this
is the correct priority for these timers, or whether we should be using a
more dynamic allocation scheme.
At the moment clock control is very basic: we just enable the clock during
probe and pin the frequency. In the future, someone could add support for
disabling the clock when not in use. Cascade mode is also unsupported.
This driver was written with reference to Xilinx DS764 for v1.03.a [1].
[1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
Signed-off-by: Sean Anderson <redacted>
---
Please let me know if I should organize this differently or if it should
be broken up.
Changes in v3:
- Add clockevent and clocksource support
- Rewrite probe to only use a device_node, since timers may need to be
initialized before we have proper devices. This does bloat the code a bit
since we can no longer rely on helpers such as dev_err_probe. We also
cannot rely on device resources being free'd on failure, so we must free
them manually.
- We now access registers through xilinx_timer_(read|write). This allows us
to deal with endianness issues, as originally seen in the microblaze
driver. CAVEAT EMPTOR: I have not tested this on big-endian!
- Remove old microblaze driver
Changes in v2:
- Don't compile this module by default for arm64
- Add dependencies on COMMON_CLK and HAS_IOMEM
- Add comment explaining why we depend on !MICROBLAZE
- Add comment describing device
- Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
- Use NSEC_TO_SEC instead of defining our own
- Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by Uwe
- Cast dividends to u64 to avoid overflow
- Check for over- and underflow when calculating TLR
- Set xilinx_pwm_ops.owner
- Don't set pwmchip.base to -1
- Check range of xlnx,count-width
- Ensure the clock is always running when the pwm is registered
- Remove debugfs file :l
- Report errors with dev_error_probe
arch/microblaze/kernel/Makefile | 2 +-
arch/microblaze/kernel/timer.c | 326 ---------------
drivers/clocksource/Kconfig | 15 +
drivers/clocksource/Makefile | 1 +
drivers/clocksource/timer-xilinx.c | 650 +++++++++++++++++++++++++++++
5 files changed, 667 insertions(+), 327 deletions(-)
delete mode 100644 arch/microblaze/kernel/timer.c
create mode 100644 drivers/clocksource/timer-xilinx.c
@@ -0,0 +1,650 @@+// SPDX-License-Identifier: GPL-2.0++/*+*Copyright(C)2021SeanAnderson<sean.anderson@seco.com>+*+*ForXilinxLogiCOREIPAXITimerdocumentation,refertoDS764:+*https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf+*+*Hardwarelimitations:+*-Whenincascademodewecannotreadthefull64-bitcounterinonego+*-Whenchangingbothdutycycleandperiod,wemayendupwithonecycle+*withtheolddutycycleandthenewperiod.+*-Cannotproduce100%dutycycle.+*-Onlyproduces"normal"output.+*/+#include<linux/bitfield.h>+#include<linux/clk.h>+#include<linux/clk-provider.h>+#include<linux/clockchips.h>+#include<linux/clocksource.h>+#include<linux/device.h>+#include<linux/interrupt.h>+#include<linux/module.h>+#include<linux/of_irq.h>+#include<linux/of_address.h>+#include<linux/platform_device.h>+#include<linux/pwm.h>+#include<linux/sched_clock.h>+#include<asm/io.h>+#if IS_ENABLED(CONFIG_MICROBLAZE)+#include<asm/cpuinfo.h>+#endif++/* A replacement for dev_err_probe, since we don't always have a device */+#define xilinx_timer_err(np, err, fmt, ...) ({ \+pr_err("%pOF: error %d: "fmt,(np),(int)(err),##__VA_ARGS__);\+err;\+})++#define TCSR0 0x00+#define TLR0 0x04+#define TCR0 0x08+#define TCSR1 0x10+#define TLR1 0x14+#define TCR1 0x18++#define TCSR_MDT BIT(0)+#define TCSR_UDT BIT(1)+#define TCSR_GENT BIT(2)+#define TCSR_CAPT BIT(3)+#define TCSR_ARHT BIT(4)+#define TCSR_LOAD BIT(5)+#define TCSR_ENIT BIT(6)+#define TCSR_ENT BIT(7)+#define TCSR_TINT BIT(8)+#define TCSR_PWMA BIT(9)+#define TCSR_ENALL BIT(10)+#define TCSR_CASC BIT(11)++/*+*TheideahereistocapturewhetherthePWMisactuallyrunning(e.g.+*becauseweorthebootloadersetitup)andweneedtobecarefultoensure+*wedon'tcauseaglitch.Accordingtothedevicedatasheet,toenablethe+*PWMweneedto+*+*-Setbothtimerstogeneratemode(MDT=1)+*-SetbothtimerstoPWMmode(PWMA=1)+*-Enablethegenerateoutsignals(GENT=1)+*+*Inaddition,+*+*-Thetimermustberunning(ENT=1)+*-Thetimermustauto-reloadTLRintoTCR(ARHT=1)+*-WemustnotbeintheprocessofloadingTLRintoTCR(LOAD=0)+*-Cascademodemustbedisabled(CASC=0)+*+*Ifanyofthesedifferfromusual,thenthePWMiseitherdisabled,oris+*runninginamodethatthisdriverdoesnotsupport.+*/+#define TCSR_PWM_SET (TCSR_GENT | TCSR_ARHT | TCSR_ENT | TCSR_PWMA)+#define TCSR_PWM_CLEAR (TCSR_MDT | TCSR_LOAD)+#define TCSR_PWM_MASK (TCSR_PWM_SET | TCSR_PWM_CLEAR)++/**+*structxilinx_timer_priv-PrivatedataforXilinxAXItimerdriver+*@cs:Clocksourcedevice+*@ce:Clockeventdevice+*@pwm:PWMcontrollerchip+*@clk:Parentclock+*@regs:Baseaddressofthisdevice+*@width:Widthofthecounters,inbits+*@XILINX_TIMER_ONE:Wehaveonlyonetimer.+*@XILINX_TIMER_PWM:ConfiguredasaPWM.+*@XILINX_TIMER_CLK:Weweremissingadevicetreeclockandcreatedourown+*@flags:Flagsforwhattypeofdeviceweare+*/+structxilinx_timer_priv{+union{+struct{+structclocksourcecs;+structclock_event_devicece;+};+structpwm_chippwm;+};+structclk*clk;+void__iomem*regs;+u32(*read)(constvolatilevoid__iomem*addr);+void(*write)(u32value,volatilevoid__iomem*addr);+unsignedintwidth;+enum{+XILINX_TIMER_ONE=BIT(0),+XILINX_TIMER_PWM=BIT(1),+XILINX_TIMER_CLK=BIT(2),+}flags;+};++staticinlinestructxilinx_timer_priv+*xilinx_pwm_chip_to_priv(structpwm_chip*chip)+{+returncontainer_of(chip,structxilinx_timer_priv,pwm);+}++staticinlinestructxilinx_timer_priv+*xilinx_clocksource_to_priv(structclocksource*cs)+{+returncontainer_of(cs,structxilinx_timer_priv,cs);+}++staticinlinestructxilinx_timer_priv+*xilinx_clockevent_to_priv(structclock_event_device*ce)+{+returncontainer_of(ce,structxilinx_timer_priv,ce);+}++staticu32xilinx_ioread32be(constvolatilevoid__iomem*addr)+{+returnioread32be(addr);+}++staticvoidxilinx_iowrite32be(u32value,volatilevoid__iomem*addr)+{+iowrite32be(value,addr);+}++staticinlineu32xilinx_timer_read(structxilinx_timer_priv*priv,+intoffset)+{+returnpriv->read(priv->regs+offset);+}++staticinlinevoidxilinx_timer_write(structxilinx_timer_priv*priv,+u32value,intoffset)+{+priv->write(value,priv->regs+offset);+}++staticinlineu64xilinx_timer_max(structxilinx_timer_priv*priv)+{+returnBIT_ULL(priv->width)-1;+}++staticintxilinx_timer_tlr_cycles(structxilinx_timer_priv*priv,u32*tlr,+u32tcsr,u64cycles)+{+u64max_count=xilinx_timer_max(priv);++if(cycles<2||cycles>max_count+2)+return-ERANGE;++if(tcsr&TCSR_UDT)+*tlr=cycles-2;+else+*tlr=max_count-cycles+2;++return0;+}++staticboolxilinx_timer_pwm_enabled(u32tcsr0,u32tcsr1)+{+return((TCSR_PWM_MASK|TCSR_CASC)&tcsr0)==TCSR_PWM_SET&&+(TCSR_PWM_MASK&tcsr1)==TCSR_PWM_SET;+}++staticintxilinx_timer_tlr_period(structxilinx_timer_priv*priv,u32*tlr,+u32tcsr,unsignedintperiod)+{+u64cycles=DIV_ROUND_DOWN_ULL((u64)period*clk_get_rate(priv->clk),+NSEC_PER_SEC);++returnxilinx_timer_tlr_cycles(priv,tlr,tcsr,cycles);+}++staticunsignedintxilinx_timer_get_period(structxilinx_timer_priv*priv,+u32tlr,u32tcsr)+{+u64cycles;++if(tcsr&TCSR_UDT)+cycles=tlr+2;+else+cycles=xilinx_timer_max(priv)-tlr+2;++returnDIV_ROUND_UP_ULL(cycles*NSEC_PER_SEC,+clk_get_rate(priv->clk));+}++staticintxilinx_pwm_apply(structpwm_chip*chip,structpwm_device*unused,+conststructpwm_state*state)+{+intret;+structxilinx_timer_priv*priv=xilinx_pwm_chip_to_priv(chip);+u32tlr0,tlr1;+u32tcsr0=xilinx_timer_read(priv,TCSR0);+u32tcsr1=xilinx_timer_read(priv,TCSR1);+boolenabled=xilinx_timer_pwm_enabled(tcsr0,tcsr1);++if(state->polarity!=PWM_POLARITY_NORMAL)+return-EINVAL;++ret=xilinx_timer_tlr_period(priv,&tlr0,tcsr0,state->period);+if(ret)+returnret;++ret=xilinx_timer_tlr_period(priv,&tlr1,tcsr1,state->duty_cycle);+if(ret)+returnret;++xilinx_timer_write(priv,tlr0,TLR0);+xilinx_timer_write(priv,tlr1,TLR1);++if(state->enabled){+/* Only touch the TCSRs if we aren't already running */+if(!enabled){+/* Load TLR into TCR */+xilinx_timer_write(priv,tcsr0|TCSR_LOAD,TCSR0);+xilinx_timer_write(priv,tcsr1|TCSR_LOAD,TCSR1);+/* Enable timers all at once with ENALL */+tcsr0=(TCSR_PWM_SET&~TCSR_ENT)|(tcsr0&TCSR_UDT);+tcsr1=TCSR_PWM_SET|TCSR_ENALL|(tcsr1&TCSR_UDT);+xilinx_timer_write(priv,tcsr0,TCSR0);+xilinx_timer_write(priv,tcsr1,TCSR1);+}+}else{+xilinx_timer_write(priv,0,TCSR0);+xilinx_timer_write(priv,0,TCSR1);+}++return0;+}++staticvoidxilinx_pwm_get_state(structpwm_chip*chip,+structpwm_device*unused,+structpwm_state*state)+{+structxilinx_timer_priv*priv=xilinx_pwm_chip_to_priv(chip);+u32tlr0=xilinx_timer_read(priv,TLR0);+u32tlr1=xilinx_timer_read(priv,TLR1);+u32tcsr0=xilinx_timer_read(priv,TCSR0);+u32tcsr1=xilinx_timer_read(priv,TCSR1);++state->period=xilinx_timer_get_period(priv,tlr0,tcsr0);+state->duty_cycle=xilinx_timer_get_period(priv,tlr1,tcsr1);+state->enabled=xilinx_timer_pwm_enabled(tcsr0,tcsr1);+state->polarity=PWM_POLARITY_NORMAL;+}++staticconststructpwm_opsxilinx_pwm_ops={+.apply=xilinx_pwm_apply,+.get_state=xilinx_pwm_get_state,+.owner=THIS_MODULE,+};++staticintxilinx_pwm_init(structdevice*dev,+structxilinx_timer_priv*priv)+{+intret;++if(!dev)+return-EPROBE_DEFER;++priv->pwm.dev=dev;+priv->pwm.ops=&xilinx_pwm_ops;+priv->pwm.npwm=1;+ret=pwmchip_add(&priv->pwm);+if(ret)+xilinx_timer_err(dev->of_node,ret,+"could not register pwm chip\n");+returnret;+}++staticirqreturn_txilinx_timer_handler(intirq,void*dev)+{+structxilinx_timer_priv*priv=dev;+u32tcsr1=xilinx_timer_read(priv,TCSR1);++/* Acknowledge interrupt */+xilinx_timer_write(priv,tcsr1|TCSR_TINT,TCSR1);+priv->ce.event_handler(&priv->ce);+returnIRQ_HANDLED;+}++staticintxilinx_clockevent_next_event(unsignedlongevt,+structclock_event_device*ce)+{+structxilinx_timer_priv*priv=xilinx_clockevent_to_priv(ce);++xilinx_timer_write(priv,evt,TLR1);+xilinx_timer_write(priv,TCSR_LOAD,TCSR1);+xilinx_timer_write(priv,TCSR_ENIT|TCSR_ENT,TCSR1);+return0;+}++staticintxilinx_clockevent_state_periodic(structclock_event_device*ce)+{+intret;+u32tlr1;+structxilinx_timer_priv*priv=xilinx_clockevent_to_priv(ce);++ret=xilinx_timer_tlr_cycles(priv,&tlr1,0,+clk_get_rate(priv->clk)/HZ);+if(ret)+returnret;++xilinx_timer_write(priv,tlr1,TLR1);+xilinx_timer_write(priv,TCSR_LOAD,TCSR1);+xilinx_timer_write(priv,TCSR_ARHT|TCSR_ENIT|TCSR_ENT,TCSR1);+return0;+}++staticintxilinx_clockevent_shutdown(structclock_event_device*ce)+{+xilinx_timer_write(xilinx_clockevent_to_priv(ce),0,TCSR1);+return0;+}++staticconststructclock_event_devicexilinx_clockevent_base={+.name="xilinx_clockevent",+.features=CLOCK_EVT_FEAT_PERIODIC|CLOCK_EVT_FEAT_ONESHOT,+.set_next_event=xilinx_clockevent_next_event,+.set_state_periodic=xilinx_clockevent_state_periodic,+.set_state_shutdown=xilinx_clockevent_shutdown,+.rating=300,+.cpumask=cpu_possible_mask,+.owner=THIS_MODULE,+};++staticintxilinx_clockevent_init(structdevice_node*np,+structxilinx_timer_priv*priv)+{+intret=of_irq_get(np,0);++if(ret<0)+returnxilinx_timer_err(np,ret,"could not get irq\n");++ret=request_irq(ret,xilinx_timer_handler,IRQF_TIMER,+np->full_name,priv);+if(ret)+returnxilinx_timer_err(np,ret,"could not request irq\n");++memcpy(&priv->ce,&xilinx_clockevent_base,sizeof(priv->ce));+clockevents_config_and_register(&priv->ce,+clk_get_rate(priv->clk),2,+min_t(u64,+xilinx_timer_max(priv)+2,+ULONG_MAX));+return0;+}++staticu64xilinx_clocksource_read(structclocksource*cs)+{+returnxilinx_timer_read(xilinx_clocksource_to_priv(cs),TCR0);+}++staticconststructclocksourcexilinx_clocksource_base={+.read=xilinx_clocksource_read,+.name="xilinx_clocksource",+.rating=300,+.flags=CLOCK_SOURCE_IS_CONTINUOUS,+.owner=THIS_MODULE,+};++staticintxilinx_clocksource_init(structxilinx_timer_priv*priv)+{+xilinx_timer_write(priv,0,TLR0);+/* Load TLR and clear any interrupts */+xilinx_timer_write(priv,TCSR_LOAD|TCSR_TINT,TCSR0);+/* Start the timer counting up with auto-reload */+xilinx_timer_write(priv,TCSR_ARHT|TCSR_ENT,TCSR0);++memcpy(&priv->cs,&xilinx_clocksource_base,sizeof(priv->cs));+priv->cs.mask=xilinx_timer_max(priv);+returnclocksource_register_hz(&priv->cs,clk_get_rate(priv->clk));+}++staticstructclk*xilinx_timer_clock_init(structdevice_node*np,+structxilinx_timer_priv*priv)+{+intret;+u32freq;+structclk_hw*hw;+structclk*clk=of_clk_get_by_name(np,"s_axi_aclk");++if(!IS_ERR(clk)||PTR_ERR(clk)==-EPROBE_DEFER)+returnclk;++pr_warn("%pOF: missing s_axi_aclk, falling back to clock-frequency\n",+np);+ret=of_property_read_u32(np,"clock-frequency",&freq);+if(ret){+#if IS_ENABLED(CONFIG_MICROBLAZE)+pr_warn("%pOF: missing clock-frequency, falling back to /cpus/timebase-frequency\n",+np);+freq=cpuinfo.cpu_clock_freq;+#else+returnERR_PTR(ret);+#endif+}++priv->flags|=XILINX_TIMER_CLK;+hw=__clk_hw_register_fixed_rate(NULL,np,"s_axi_aclk",NULL,NULL,+NULL,0,freq,0,0);+if(IS_ERR(hw))+returnERR_CAST(hw);+returnhw->clk;+}++staticstructxilinx_timer_priv*xilinx_timer_init(structdevice*dev,+structdevice_node*np)+{+boolpwm;+inti,ret;+structxilinx_timer_priv*priv;+u32one_timer,tcsr0;++priv=kzalloc(sizeof(*priv),GFP_KERNEL);+if(!priv)+returnERR_PTR(-ENOMEM);++priv->regs=of_iomap(np,0);+if(!priv->regs){+ret=-ENXIO;+gotoerr_priv;+}elseif(IS_ERR(priv->regs)){+ret=PTR_ERR(priv->regs);+gotoerr_priv;+}++priv->read=ioread32;+priv->write=iowrite32;+/*+*Wearen'tusingtheinterruptsyet,souseENITtodetectendianness+*/+tcsr0=xilinx_timer_read(priv,TCSR0);+if(swab32(tcsr0)&TCSR_ENIT){+ret=xilinx_timer_err(np,-EOPNOTSUPP,+"cannot determine endianness\n");+gotoerr_priv;+}++xilinx_timer_write(priv,tcsr0|TCSR_ENIT,TCSR0);+if(!(xilinx_timer_read(priv,TCSR0)&TCSR_ENIT)){+priv->read=xilinx_ioread32be;+priv->write=xilinx_iowrite32be;+}++/*+*Forbackwardscompatibility,allowxlnx,one-timer-only=<bool>;+*However,thepreferredwayistousethexlnx,single-timerflag.+*/+one_timer=of_property_read_bool(np,"xlnx,single-timer");+if(!one_timer){+ret=of_property_read_u32(np,"xlnx,one-timer-only",&one_timer);+if(ret){+ret=xilinx_timer_err(np,ret,"xlnx,one-timer-only");+gotoerr_priv;+}+}++pwm=of_property_read_bool(np,"xlnx,pwm");+if(one_timer&&pwm){+ret=xilinx_timer_err(np,-EINVAL,+"pwm mode not possible with one timer\n");+gotoerr_priv;+}++priv->flags=FIELD_PREP(XILINX_TIMER_ONE,one_timer)|+FIELD_PREP(XILINX_TIMER_PWM,pwm);++for(i=0;pwm&&i<2;i++){+charint_fmt[]="xlnx,gen%u-assert";+charbool_fmt[]="xlnx,gen%u-active-low";+charbuf[max(sizeof(int_fmt),sizeof(bool_fmt))];+u32gen;++/*+*Allowxlnx,gen?-assert=<bool>;forbackwards+*compatibility.However,thepreferredwayistousethe+*xlnx,gen?-active-lowflag.+*/+snprintf(buf,sizeof(buf),bool_fmt,i);+gen=!of_property_read_bool(np,buf);+if(gen){+snprintf(buf,sizeof(buf),int_fmt,i);+ret=of_property_read_u32(np,buf,&gen);+if(ret&&ret!=-EINVAL){+xilinx_timer_err(np,ret,"%s\n",buf);+gotoerr_priv;+}+}++if(!gen){+ret=xilinx_timer_err(np,-EINVAL,+"generateout%u must be active high\n",+i);+gotoerr_priv;+}+}++ret=of_property_read_u32(np,"xlnx,count-width",&priv->width);+if(ret){+xilinx_timer_err(np,ret,"xlnx,count-width\n");+gotoerr_priv;+}elseif(priv->width<8||priv->width>32){+ret=xilinx_timer_err(np,-EINVAL,"invalid counter width\n");+gotoerr_priv;+}++priv->clk=xilinx_timer_clock_init(np,priv);+if(IS_ERR(priv->clk)){+ret=xilinx_timer_err(np,PTR_ERR(priv->clk),"clock\n");+gotoerr_priv;+}++ret=clk_prepare_enable(priv->clk);+if(ret){+xilinx_timer_err(np,ret,"clock enable failed\n");+gotoerr_clk;+}+clk_rate_exclusive_get(priv->clk);++if(pwm){+ret=xilinx_pwm_init(dev,priv);+}else{+ret=xilinx_clocksource_init(priv);+if(!ret&&!one_timer){+ret=xilinx_clockevent_init(np,priv);+if(ret)+priv->flags|=XILINX_TIMER_ONE;+}+}++if(!ret)+returnpriv;++clk_rate_exclusive_put(priv->clk);+clk_disable_unprepare(priv->clk);+err_clk:+if(priv->flags&XILINX_TIMER_CLK)+clk_unregister_fixed_rate(priv->clk);+else+clk_put(priv->clk);+err_priv:+kfree(priv);+returnERR_PTR(ret);+}++staticintxilinx_timer_probe(structplatform_device*pdev)+{+structxilinx_timer_priv*priv=+xilinx_timer_init(&pdev->dev,pdev->dev.of_node);++if(IS_ERR(priv))+returnPTR_ERR(priv);++platform_set_drvdata(pdev,priv);+return0;+}++staticintxilinx_timer_remove(structplatform_device*pdev)+{+structxilinx_timer_priv*priv=platform_get_drvdata(pdev);++if(IS_ENABLED(CONFIG_XILINX_PWM)&&priv->flags&XILINX_TIMER_PWM){+pwmchip_remove(&priv->pwm);+}else{+if(!(priv->flags&XILINX_TIMER_ONE)){+intcpu;++for_each_cpu(cpu,priv->ce.cpumask)+clockevents_unbind_device(&priv->ce,cpu);+}+clocksource_unregister(&priv->cs);+}++clk_rate_exclusive_put(priv->clk);+clk_disable_unprepare(priv->clk);+if(priv->flags&XILINX_TIMER_CLK)+clk_unregister_fixed_rate(priv->clk);+else+clk_put(priv->clk);+return0;+}++staticconststructof_device_idxilinx_timer_of_match[]={+{.compatible="xlnx,xps-timer-1.00.a",},+{.compatible="xlnx,axi-timer-2.0"},+{},+};+MODULE_DEVICE_TABLE(of,xilinx_timer_of_match);++staticstructplatform_driverxilinx_timer_driver={+.probe=xilinx_timer_probe,+.remove=xilinx_timer_remove,+.driver={+.name="xilinx-timer",+.of_match_table=of_match_ptr(xilinx_timer_of_match),+},+};+module_platform_driver(xilinx_timer_driver);++staticstructxilinx_timer_priv*xilinx_sched=(void*)-EAGAIN;++staticu64xilinx_sched_read(void)+{+returnxilinx_timer_read(xilinx_sched,TCSR0);
drivers/clocksource/timer-xilinx.c:136:20: warning: passing argument 1 of 'ioread32be' discards 'volatile' qualifier from pointer target type [-Wdiscarded-qualifiers]
136 | return ioread32be(addr);
| ^~~~
In file included from arch/mips/include/asm/io.h:28,
from arch/mips/include/asm/mmiowb.h:5,
from include/linux/spinlock.h:61,
from include/linux/rwsem.h:16,
from include/linux/notifier.h:15,
from include/linux/clk.h:14,
from drivers/clocksource/timer-xilinx.c:16:
include/asm-generic/iomap.h:33:32: note: expected 'const void *' but argument is of type 'const volatile void *'
33 | extern unsigned int ioread32be(const void __iomem *);
| ^~~~~~~~~~~~~~~~~~~~
drivers/clocksource/timer-xilinx.c: In function 'xilinx_iowrite32be':
quoted
drivers/clocksource/timer-xilinx.c:141:21: warning: passing argument 2 of 'iowrite32be' discards 'volatile' qualifier from pointer target type [-Wdiscarded-qualifiers]
141 | iowrite32be(value, addr);
| ^~~~
In file included from arch/mips/include/asm/io.h:28,
from arch/mips/include/asm/mmiowb.h:5,
from include/linux/spinlock.h:61,
from include/linux/rwsem.h:16,
from include/linux/notifier.h:15,
from include/linux/clk.h:14,
from drivers/clocksource/timer-xilinx.c:16:
include/asm-generic/iomap.h:54:30: note: expected 'void *' but argument is of type 'volatile void *'
54 | extern void iowrite32be(u32, void __iomem *);
| ^~~~~~~~~~~~~~
drivers/clocksource/timer-xilinx.c: In function 'xilinx_timer_init':
quoted
drivers/clocksource/timer-xilinx.c:447:13: error: assignment to 'u32 (*)(const volatile void *)' {aka 'unsigned int (*)(const volatile void *)'} from incompatible pointer type 'unsigned int (*)(const void *)' [-Werror=incompatible-pointer-types]
From: Rob Herring <robh@kernel.org> Date: 2021-05-12 20:34:40
On Tue, 11 May 2021 15:12:37 -0400, Sean Anderson wrote:
This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
a "soft" block, so it has many parameters which would not be
configurable in most hardware. This binding is usually automatically
generated by Xilinx's tools, so the names and values of some properties
must be kept as they are. Replacement properties have been provided for
new device trees.
Because we need to init timer devices so early in boot, the easiest way
to configure things is to use a device tree property. For the moment
this is 'xlnx,pwm', but this could be extended/renamed/etc. in the
future if these is a need for a generic property.
Signed-off-by: Sean Anderson <redacted>
---
How should the clocking situation be documented? For the moment I have
just left clock as optional, but should clock-frequency be documented?
Changes in v3:
- Mark all boolean-as-int properties as deprecated
- Add xlnx,pwm and xlnx,gen?-active-low properties.
- Make newer replacement properties mutually-exclusive with what they
replace
- Add an example with non-deprecated properties only.
Changes in v2:
- Use 32-bit addresses for example binding
.../bindings/pwm/xlnx,axi-timer.yaml | 142 ++++++++++++++++++
1 file changed, 142 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
./Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml:16:9: [warning] wrong indentation: expected 10 but found 8 (indentation)
dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/pwm/xlnx,axi-timer.example.dts:49.37-57.11: ERROR (duplicate_label): /example-1/timer@800e0000: Duplicate label 'axi_timer_0' on /example-1/timer@800e0000 and /example-0/timer@800e0000
ERROR: Input tree has errors, aborting (use -f to force output)
make[1]: *** [scripts/Makefile.lib:380: Documentation/devicetree/bindings/pwm/xlnx,axi-timer.example.dt.yaml] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1416: dt_binding_check] Error 2
See https://patchwork.ozlabs.org/patch/1477288
This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Rob Herring <robh@kernel.org> Date: 2021-05-13 02:16:35
On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
a "soft" block, so it has many parameters which would not be
configurable in most hardware. This binding is usually automatically
generated by Xilinx's tools, so the names and values of some properties
must be kept as they are. Replacement properties have been provided for
new device trees.
Because you have some tool generating properties is not a reason we have
to accept them upstream. 'deprecated' is for what *we* have deprecated.
In this case, I don't really see the point in defining new properties
just to have bool.
Because we need to init timer devices so early in boot, the easiest way
to configure things is to use a device tree property. For the moment
this is 'xlnx,pwm', but this could be extended/renamed/etc. in the
future if these is a need for a generic property.
No...
quoted hunk
Signed-off-by: Sean Anderson <redacted>
---
How should the clocking situation be documented? For the moment I have
just left clock as optional, but should clock-frequency be documented?
Changes in v3:
- Mark all boolean-as-int properties as deprecated
- Add xlnx,pwm and xlnx,gen?-active-low properties.
- Make newer replacement properties mutually-exclusive with what they
replace
- Add an example with non-deprecated properties only.
Changes in v2:
- Use 32-bit addresses for example binding
.../bindings/pwm/xlnx,axi-timer.yaml | 142 ++++++++++++++++++
1 file changed, 142 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
@@ -0,0 +1,142 @@+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause+%YAML1.2+---+$id:http://devicetree.org/schemas/pwm/xlnx,axi-timer.yaml#+$schema:http://devicetree.org/meta-schemas/core.yaml#++title:Xilinx LogiCORE IP AXI Timer Device Tree Binding++maintainers:+-Sean Anderson <sean.anderson@seco.com>++properties:+compatible:+oneOf:+-items:+-const:xlnx,axi-timer-2.0+-const:xlnx,xps-timer-1.00.a+-const:xlnx,xps-timer-1.00.a++clocks:+maxItems:1++clock-names:+const:s_axi_aclk++reg:+maxItems:1++xlnx,count-width:+$ref:/schemas/types.yaml#/definitions/uint32+minimum:8+maximum:32+description:+The width of the counter(s), in bits.++xlnx,gen0-assert:+$ref:/schemas/types.yaml#/definitions/uint32+enum:[0,1]+default:1+deprecated:true+description:+The polarity of the generateout0 signal. 0 for active-low, 1 for active-high.++xlnx,gen0-active-low:+$ref:/schemas/types.yaml#/definitions/flag+description:+The generate0 signal is active-low.++xlnx,gen1-assert:+$ref:/schemas/types.yaml#/definitions/uint32+enum:[0,1]+default:1+deprecated:true+description:+The polarity of the generateout1 signal. 0 for active-low, 1 for active-high.++xlnx,gen1-active-low:+$ref:/schemas/types.yaml#/definitions/flag+description:+The generate1 signal is active-low.++xlnx,one-timer-only:+$ref:/schemas/types.yaml#/definitions/uint32+enum:[0,1]+deprecated:true+description:+Whether only one timer is present in this block.++xlnx,single-timer:+$ref:/schemas/types.yaml#/definitions/flag+description:+Only one timer is present in this block.++xlnx,pwm:+$ref:/schemas/types.yaml#/definitions/flag+description:+This timer should be configured as a PWM.
If a PWM, perhaps you want a '#pwm-cells' property which can serve as
the hint to configure as a PWM.
From: Sean Anderson <hidden> Date: 2021-05-13 14:34:11
On 5/12/21 10:16 PM, Rob Herring wrote:
> On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
>> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
>> a "soft" block, so it has many parameters which would not be
>> configurable in most hardware. This binding is usually automatically
>> generated by Xilinx's tools, so the names and values of some properties
>> must be kept as they are. Replacement properties have been provided for
>> new device trees.
>
> Because you have some tool generating properties is not a reason we have
> to accept them upstream.
These properties are already in arch/microblaze/boot/dts/system.dts and
in the devicetree supplied to Linux by qemu. Removing these properties
will break existing setups, which I would like to avoid.
> 'deprecated' is for what *we* have deprecated.
Ok. I will remove that then.
>
> In this case, I don't really see the point in defining new properties
> just to have bool.
I don't either, but it was requested, by Michal...
>
>>
>> Because we need to init timer devices so early in boot, the easiest way
>> to configure things is to use a device tree property. For the moment
>> this is 'xlnx,pwm', but this could be extended/renamed/etc. in the
>> future if these is a need for a generic property.
>
> No...
>
>> Signed-off-by: Sean Anderson [off-list ref]
>> ---
>> How should the clocking situation be documented? For the moment I have
>> just left clock as optional, but should clock-frequency be documented?
>>
>> Changes in v3:
>> - Mark all boolean-as-int properties as deprecated
>> - Add xlnx,pwm and xlnx,gen?-active-low properties.
>> - Make newer replacement properties mutually-exclusive with what they
>> replace
>> - Add an example with non-deprecated properties only.
>>
>> Changes in v2:
>> - Use 32-bit addresses for example binding
>>
>> .../bindings/pwm/xlnx,axi-timer.yaml | 142 ++++++++++++++++++
>> 1 file changed, 142 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
>> new file mode 100644
>> index 000000000000..a5e90658e31a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
>> @@ -0,0 +1,142 @@
>> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pwm/xlnx,axi-timer.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Xilinx LogiCORE IP AXI Timer Device Tree Binding
>> +
>> +maintainers:
>> + - Sean Anderson [off-list ref]
>> +
>> +properties:
>> + compatible:
>> + oneOf:
>> + - items:
>> + - const: xlnx,axi-timer-2.0
>> + - const: xlnx,xps-timer-1.00.a
>> + - const: xlnx,xps-timer-1.00.a
>> +
>> + clocks:
>> + maxItems: 1
>> +
>> + clock-names:
>> + const: s_axi_aclk
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + xlnx,count-width:
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + minimum: 8
>> + maximum: 32
>> + description:
>> + The width of the counter(s), in bits.
>> +
>> + xlnx,gen0-assert:
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + enum: [ 0, 1 ]
>> + default: 1
>> + deprecated: true
>> + description:
>> + The polarity of the generateout0 signal. 0 for active-low, 1 for active-high.
>> +
>> + xlnx,gen0-active-low:
>> + $ref: /schemas/types.yaml#/definitions/flag
>> + description:
>> + The generate0 signal is active-low.
>> +
>> + xlnx,gen1-assert:
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + enum: [ 0, 1 ]
>> + default: 1
>> + deprecated: true
>> + description:
>> + The polarity of the generateout1 signal. 0 for active-low, 1 for active-high.
>> +
>> + xlnx,gen1-active-low:
>> + $ref: /schemas/types.yaml#/definitions/flag
>> + description:
>> + The generate1 signal is active-low.
>> +
>> + xlnx,one-timer-only:
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + enum: [ 0, 1 ]
>> + deprecated: true
>> + description:
>> + Whether only one timer is present in this block.
>> +
>> + xlnx,single-timer:
>> + $ref: /schemas/types.yaml#/definitions/flag
>> + description:
>> + Only one timer is present in this block.
>> +
>> + xlnx,pwm:
>> + $ref: /schemas/types.yaml#/definitions/flag
>> + description:
>> + This timer should be configured as a PWM.
>
> If a PWM, perhaps you want a '#pwm-cells' property which can serve as
> the hint to configure as a PWM.
Ok, that's a good idea.
--Sean
>
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - xlnx,count-width
>> +
>> +allOf:
>> + - if:
>> + required:
>> + - clocks
>> + then:
>> + required:
>> + - clock-names
>> +
>> + - if:
>> + required:
>> + - xlnx,gen0-active-low
>> + then:
>> + not:
>> + required:
>> + - xlnx,gen0-assert
>> +
>> + - if:
>> + required:
>> + - xlnx,gen0-active-low
>> + then:
>> + not:
>> + required:
>> + - xlnx,gen0-assert
>> +
>> + - if:
>> + required:
>> + - xlnx,one-timer-only
>> + then:
>> + not:
>> + required:
>> + - xlnx,single-timer
>> +
>> +additionalProperties: true
>> +
>> +examples:
>> + - |
>> + axi_timer_0: timer@800e0000 {
>> + clock-names = "s_axi_aclk";
>> + clocks = <&zynqmp_clk 71>;
>> + compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";
>> + reg = <0x800e0000 0x10000>;
>> + xlnx,count-width = <0x20>;
>> + xlnx,gen0-assert = <0x1>;
>> + xlnx,gen1-assert = <0x1>;
>> + xlnx,one-timer-only = <0x0>;
>> + xlnx,trig0-assert = <0x1>;
>> + xlnx,trig1-assert = <0x1>;
>> + };
>> +
>> + - |
>> + axi_timer_0: timer@800e0000 {
>> + clock-names = "s_axi_aclk";
>> + clocks = <&zynqmp_clk 71>;
>> + compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";
>> + reg = <0x800e0000 0x10000>;
>> + xlnx,count-width = <0x20>;
>> + xlnx,gen0-active-low;
>> + xlnx,single-timer;
>> + };
>> --
>> 2.25.1
>>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Sean Anderson <hidden> Date: 2021-05-13 15:28:45
On 5/13/21 10:33 AM, Sean Anderson wrote:
>
>
> On 5/12/21 10:16 PM, Rob Herring wrote:
> > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
> >> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
> >> a "soft" block, so it has many parameters which would not be
> >> configurable in most hardware. This binding is usually automatically
> >> generated by Xilinx's tools, so the names and values of some properties
> >> must be kept as they are. Replacement properties have been provided for
> >> new device trees.
> >
> > Because you have some tool generating properties is not a reason we have
> > to accept them upstream.
>
> These properties are already in arch/microblaze/boot/dts/system.dts and
> in the devicetree supplied to Linux by qemu. Removing these properties
> will break existing setups, which I would like to avoid.
>
> > 'deprecated' is for what *we* have deprecated.
>
> Ok. I will remove that then.
>
> >
> > In this case, I don't really see the point in defining new properties
> > just to have bool.
>
> I don't either, but it was requested, by Michal...
Err, your comment on the original bindings was
> Can't all these be boolean?
And Michal commented
> I think in this case you should described what it is used by current
> driver in Microblaze and these options are required. The rest are by
> design optional.
> If you want to change them to different value then current binding
> should be deprecated and have any transition time with code alignment.
So that is what I tried to accomplish with this revision. I also tried
allowing something like
xlnx,one-timer-only = <0>; /* two timers */
xlnx,one-timer-only = <1>; /* one timer */
xlnx,one-timer-only; /* one timer */
/* property absent means two timers */
but I was unable to figure out how to express this with json-schema. I
don't think it's the best design either...
--Sean
>
> >
> >>
> >> Because we need to init timer devices so early in boot, the easiest way
> >> to configure things is to use a device tree property. For the moment
> >> this is 'xlnx,pwm', but this could be extended/renamed/etc. in the
> >> future if these is a need for a generic property.
> >
> > No...
> >
> >> Signed-off-by: Sean Anderson [off-list ref]
> >> ---
> >> How should the clocking situation be documented? For the moment I have
> >> just left clock as optional, but should clock-frequency be documented?
> >>
> >> Changes in v3:
> >> - Mark all boolean-as-int properties as deprecated
> >> - Add xlnx,pwm and xlnx,gen?-active-low properties.
> >> - Make newer replacement properties mutually-exclusive with what they
> >> replace
> >> - Add an example with non-deprecated properties only.
> >>
> >> Changes in v2:
> >> - Use 32-bit addresses for example binding
> >>
> >> .../bindings/pwm/xlnx,axi-timer.yaml | 142 ++++++++++++++++++
> >> 1 file changed, 142 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
> >> new file mode 100644
> >> index 000000000000..a5e90658e31a
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
> >> @@ -0,0 +1,142 @@
> >> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/pwm/xlnx,axi-timer.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Xilinx LogiCORE IP AXI Timer Device Tree Binding
> >> +
> >> +maintainers:
> >> + - Sean Anderson [off-list ref]
> >> +
> >> +properties:
> >> + compatible:
> >> + oneOf:
> >> + - items:
> >> + - const: xlnx,axi-timer-2.0
> >> + - const: xlnx,xps-timer-1.00.a
> >> + - const: xlnx,xps-timer-1.00.a
> >> +
> >> + clocks:
> >> + maxItems: 1
> >> +
> >> + clock-names:
> >> + const: s_axi_aclk
> >> +
> >> + reg:
> >> + maxItems: 1
> >> +
> >> + xlnx,count-width:
> >> + $ref: /schemas/types.yaml#/definitions/uint32
> >> + minimum: 8
> >> + maximum: 32
> >> + description:
> >> + The width of the counter(s), in bits.
> >> +
> >> + xlnx,gen0-assert:
> >> + $ref: /schemas/types.yaml#/definitions/uint32
> >> + enum: [ 0, 1 ]
> >> + default: 1
> >> + deprecated: true
> >> + description:
> >> + The polarity of the generateout0 signal. 0 for active-low, 1 for active-high.
> >> +
> >> + xlnx,gen0-active-low:
> >> + $ref: /schemas/types.yaml#/definitions/flag
> >> + description:
> >> + The generate0 signal is active-low.
> >> +
> >> + xlnx,gen1-assert:
> >> + $ref: /schemas/types.yaml#/definitions/uint32
> >> + enum: [ 0, 1 ]
> >> + default: 1
> >> + deprecated: true
> >> + description:
> >> + The polarity of the generateout1 signal. 0 for active-low, 1 for active-high.
> >> +
> >> + xlnx,gen1-active-low:
> >> + $ref: /schemas/types.yaml#/definitions/flag
> >> + description:
> >> + The generate1 signal is active-low.
> >> +
> >> + xlnx,one-timer-only:
> >> + $ref: /schemas/types.yaml#/definitions/uint32
> >> + enum: [ 0, 1 ]
> >> + deprecated: true
> >> + description:
> >> + Whether only one timer is present in this block.
> >> +
> >> + xlnx,single-timer:
> >> + $ref: /schemas/types.yaml#/definitions/flag
> >> + description:
> >> + Only one timer is present in this block.
> >> +
> >> + xlnx,pwm:
> >> + $ref: /schemas/types.yaml#/definitions/flag
> >> + description:
> >> + This timer should be configured as a PWM.
> >
> > If a PWM, perhaps you want a '#pwm-cells' property which can serve as
> > the hint to configure as a PWM.
>
> Ok, that's a good idea.
>
> --Sean
>
> >
> >> +
> >> +required:
> >> + - compatible
> >> + - reg
> >> + - xlnx,count-width
> >> +
> >> +allOf:
> >> + - if:
> >> + required:
> >> + - clocks
> >> + then:
> >> + required:
> >> + - clock-names
> >> +
> >> + - if:
> >> + required:
> >> + - xlnx,gen0-active-low
> >> + then:
> >> + not:
> >> + required:
> >> + - xlnx,gen0-assert
> >> +
> >> + - if:
> >> + required:
> >> + - xlnx,gen0-active-low
> >> + then:
> >> + not:
> >> + required:
> >> + - xlnx,gen0-assert
> >> +
> >> + - if:
> >> + required:
> >> + - xlnx,one-timer-only
> >> + then:
> >> + not:
> >> + required:
> >> + - xlnx,single-timer
> >> +
> >> +additionalProperties: true
> >> +
> >> +examples:
> >> + - |
> >> + axi_timer_0: timer@800e0000 {
> >> + clock-names = "s_axi_aclk";
> >> + clocks = <&zynqmp_clk 71>;
> >> + compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";
> >> + reg = <0x800e0000 0x10000>;
> >> + xlnx,count-width = <0x20>;
> >> + xlnx,gen0-assert = <0x1>;
> >> + xlnx,gen1-assert = <0x1>;
> >> + xlnx,one-timer-only = <0x0>;
> >> + xlnx,trig0-assert = <0x1>;
> >> + xlnx,trig1-assert = <0x1>;
> >> + };
> >> +
> >> + - |
> >> + axi_timer_0: timer@800e0000 {
> >> + clock-names = "s_axi_aclk";
> >> + clocks = <&zynqmp_clk 71>;
> >> + compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";
> >> + reg = <0x800e0000 0x10000>;
> >> + xlnx,count-width = <0x20>;
> >> + xlnx,gen0-active-low;
> >> + xlnx,single-timer;
> >> + };
> >> --
> >> 2.25.1
> >>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Rob Herring <robh@kernel.org> Date: 2021-05-13 20:43:41
On Thu, May 13, 2021 at 10:28 AM Sean Anderson [off-list ref] wrote:
On 5/13/21 10:33 AM, Sean Anderson wrote:
>
>
> On 5/12/21 10:16 PM, Rob Herring wrote:
> > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
> >> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
> >> a "soft" block, so it has many parameters which would not be
> >> configurable in most hardware. This binding is usually automatically
> >> generated by Xilinx's tools, so the names and values of some properties
> >> must be kept as they are. Replacement properties have been provided for
> >> new device trees.
> >
> > Because you have some tool generating properties is not a reason we have
> > to accept them upstream.
>
> These properties are already in arch/microblaze/boot/dts/system.dts and
> in the devicetree supplied to Linux by qemu. Removing these properties
> will break existing setups, which I would like to avoid.
Already in use in upstream dts files is different than just
'automatically generated' by vendor tools.
>
> > 'deprecated' is for what *we* have deprecated.
>
> Ok. I will remove that then.
>
> >
> > In this case, I don't really see the point in defining new properties
> > just to have bool.
>
> I don't either, but it was requested, by Michal...
Err, your comment on the original bindings was
> Can't all these be boolean?
With no other context, yes that's what I would ask. Now you've given
me some context, between using the existing ones and 2 sets of
properties to maintain, I choose the former.
And Michal commented
> I think in this case you should described what it is used by current
> driver in Microblaze and these options are required. The rest are by
> design optional.
> If you want to change them to different value then current binding
> should be deprecated and have any transition time with code alignment.
So that is what I tried to accomplish with this revision. I also tried
allowing something like
xlnx,one-timer-only = <0>; /* two timers */
xlnx,one-timer-only = <1>; /* one timer */
xlnx,one-timer-only; /* one timer */
/* property absent means two timers */
but I was unable to figure out how to express this with json-schema. I
don't think it's the best design either...
json-schema would certainly let you, but generally we don't want
properties to have more than 1 type.
Rob
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Sean Anderson <hidden> Date: 2021-05-13 21:01:14
On 5/13/21 4:43 PM, Rob Herring wrote:
> On Thu, May 13, 2021 at 10:28 AM Sean Anderson [off-list ref] wrote:
>>
>>
>>
>> On 5/13/21 10:33 AM, Sean Anderson wrote:
>> >
>> >
>> > On 5/12/21 10:16 PM, Rob Herring wrote:
>> > > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
>> > >> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
>> > >> a "soft" block, so it has many parameters which would not be
>> > >> configurable in most hardware. This binding is usually automatically
>> > >> generated by Xilinx's tools, so the names and values of some properties
>> > >> must be kept as they are. Replacement properties have been provided for
>> > >> new device trees.
>> > >
>> > > Because you have some tool generating properties is not a reason we have
>> > > to accept them upstream.
>> >
>> > These properties are already in arch/microblaze/boot/dts/system.dts and
>> > in the devicetree supplied to Linux by qemu. Removing these properties
>> > will break existing setups, which I would like to avoid.
>
> Already in use in upstream dts files is different than just
> 'automatically generated' by vendor tools.
>
>> >
>> > > 'deprecated' is for what *we* have deprecated.
>> >
>> > Ok. I will remove that then.
>> >
>> > >
>> > > In this case, I don't really see the point in defining new properties
>> > > just to have bool.
>> >
>> > I don't either, but it was requested, by Michal...
>>
>> Err, your comment on the original bindings was
>>
>> > Can't all these be boolean?
>
> With no other context, yes that's what I would ask. Now you've given
> me some context, between using the existing ones and 2 sets of
> properties to maintain, I choose the former.
Ok, then I will go with that for v4.
I noticed some another patch regarding a similar situation [1]. Would
you prefer separate files like what is done there, or is a unified file
like this ok?
[1] https://lore.kernel.org/lkml/d36e3690ce8c5a1e53d054552e4fd8b90d6a5478.1620648868.git.geert+renesas@glider.be/T/
--Sean
>
>> And Michal commented
>>
>> > I think in this case you should described what it is used by current
>> > driver in Microblaze and these options are required. The rest are by
>> > design optional.
>> > If you want to change them to different value then current binding
>> > should be deprecated and have any transition time with code alignment.
>>
>> So that is what I tried to accomplish with this revision. I also tried
>> allowing something like
>>
>> xlnx,one-timer-only = <0>; /* two timers */
>> xlnx,one-timer-only = <1>; /* one timer */
>> xlnx,one-timer-only; /* one timer */
>> /* property absent means two timers */
>>
>> but I was unable to figure out how to express this with json-schema. I
>> don't think it's the best design either...
>
> json-schema would certainly let you, but generally we don't want
> properties to have more than 1 type.
>
> Rob
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Michal Simek <hidden> Date: 2021-05-14 08:50:46
On 5/13/21 10:43 PM, Rob Herring wrote:
On Thu, May 13, 2021 at 10:28 AM Sean Anderson [off-list ref] wrote:
quoted
On 5/13/21 10:33 AM, Sean Anderson wrote:
>
>
> On 5/12/21 10:16 PM, Rob Herring wrote:
> > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
> >> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
> >> a "soft" block, so it has many parameters which would not be
> >> configurable in most hardware. This binding is usually automatically
> >> generated by Xilinx's tools, so the names and values of some properties
> >> must be kept as they are. Replacement properties have been provided for
> >> new device trees.
> >
> > Because you have some tool generating properties is not a reason we have
> > to accept them upstream.
>
> These properties are already in arch/microblaze/boot/dts/system.dts and
> in the devicetree supplied to Linux by qemu. Removing these properties
> will break existing setups, which I would like to avoid.
Already in use in upstream dts files is different than just
'automatically generated' by vendor tools.
quoted
>
> > 'deprecated' is for what *we* have deprecated.
>
> Ok. I will remove that then.
>
> >
> > In this case, I don't really see the point in defining new properties
> > just to have bool.
>
> I don't either, but it was requested, by Michal...
Err, your comment on the original bindings was
> Can't all these be boolean?
With no other context, yes that's what I would ask. Now you've given
me some context, between using the existing ones and 2 sets of
properties to maintain, I choose the former.
quoted
And Michal commented
> I think in this case you should described what it is used by current
> driver in Microblaze and these options are required. The rest are by
> design optional.
> If you want to change them to different value then current binding
> should be deprecated and have any transition time with code alignment.
So that is what I tried to accomplish with this revision. I also tried
allowing something like
xlnx,one-timer-only = <0>; /* two timers */
xlnx,one-timer-only = <1>; /* one timer */
xlnx,one-timer-only; /* one timer */
/* property absent means two timers */
but I was unable to figure out how to express this with json-schema. I
don't think it's the best design either...
json-schema would certainly let you, but generally we don't want
properties to have more than 1 type.
One thing is what it is in system.dts file which was committed in 2009
and there are just small alignments there. But none is really using it.
Maybe I should just delete it.
And this version was generated by Xilinx ancient tools at that time. All
parameters there are fully describing HW and they are not changing. Only
new one can be added.
From the current microblaze code you can see which properties are really
used.
reg
interrupts
xlnx,one-timer-only
clocks
clock-frequency
It means from my point of view these should be listed in the binding.
clock-frequency is optional by code when clock is defined.
All other properties listed in system.dts are from my perspective
optional and that's how it should be.
I think DT binding patch should reflect this state as patch itself.
And then PWM should be added on the top as separate patch.
Note: In past we were using only parameters and name we got from tools
but over years we were fine to use for example bool properties and we
just aligned Xilinx device tree generator to match it. That's why not a
problem to deprecate any property and move to new one. Xilinx DTG is
already prepared for it and it is easy to remap it.
Thanks,
Michal
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Michal Simek <hidden> Date: 2021-05-14 08:59:43
On 5/11/21 9:12 PM, Sean Anderson wrote:
This adds generic clocksource and clockevent support for Xilinx LogiCORE IP
AXI soft timers commonly found on Xilinx FPGAs. This timer is also the
primary timer for Microblaze processors. This commit also adds support for
configuring this timer as a PWM (though this could be split off if
necessary). This whole driver lives in clocksource because it is primarily
clocksource stuff now (even though it started out as a PWM driver). I think
teasing apart the driver would not be worth it since they share so many
functions.
This driver configures timer 0 (which is always present) as a clocksource,
and timer 1 (which might be missing) as a clockevent. I don't know if this
is the correct priority for these timers, or whether we should be using a
more dynamic allocation scheme.
At the moment clock control is very basic: we just enable the clock during
probe and pin the frequency. In the future, someone could add support for
disabling the clock when not in use. Cascade mode is also unsupported.
This driver was written with reference to Xilinx DS764 for v1.03.a [1].
[1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
Signed-off-by: Sean Anderson <redacted>
---
Please let me know if I should organize this differently or if it should
be broken up.
Changes in v3:
- Add clockevent and clocksource support
- Rewrite probe to only use a device_node, since timers may need to be
initialized before we have proper devices. This does bloat the code a bit
since we can no longer rely on helpers such as dev_err_probe. We also
cannot rely on device resources being free'd on failure, so we must free
them manually.
- We now access registers through xilinx_timer_(read|write). This allows us
to deal with endianness issues, as originally seen in the microblaze
driver. CAVEAT EMPTOR: I have not tested this on big-endian!
- Remove old microblaze driver
Changes in v2:
- Don't compile this module by default for arm64
- Add dependencies on COMMON_CLK and HAS_IOMEM
- Add comment explaining why we depend on !MICROBLAZE
- Add comment describing device
- Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
- Use NSEC_TO_SEC instead of defining our own
- Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by Uwe
- Cast dividends to u64 to avoid overflow
- Check for over- and underflow when calculating TLR
- Set xilinx_pwm_ops.owner
- Don't set pwmchip.base to -1
- Check range of xlnx,count-width
- Ensure the clock is always running when the pwm is registered
- Remove debugfs file :l
- Report errors with dev_error_probe
arch/microblaze/kernel/Makefile | 2 +-
arch/microblaze/kernel/timer.c | 326 ---------------
drivers/clocksource/Kconfig | 15 +
drivers/clocksource/Makefile | 1 +
drivers/clocksource/timer-xilinx.c | 650 +++++++++++++++++++++++++++++
5 files changed, 667 insertions(+), 327 deletions(-)
delete mode 100644 arch/microblaze/kernel/timer.c
create mode 100644 drivers/clocksource/timer-xilinx.c
I don't think this is the right way to go.
The first patch should be move current timer driver from microblaze to
generic location and then apply patches on the top based on what you are
adding/fixing to be able to review every change separately.
When any issue happens it can be bisected and exact patch is identified.
With this way we will end up in this patch and it will take a lot of
time to find where that problem is.
Another part of this is that you have c&p some parts from origin driver
and do not keep origin authors there which can be consider as license
violation.
Thanks,
Michal
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Sean Anderson <hidden> Date: 2021-05-14 14:40:38
On 5/14/21 4:59 AM, Michal Simek wrote:
>
>
> On 5/11/21 9:12 PM, Sean Anderson wrote:
>> This adds generic clocksource and clockevent support for Xilinx LogiCORE IP
>> AXI soft timers commonly found on Xilinx FPGAs. This timer is also the
>> primary timer for Microblaze processors. This commit also adds support for
>> configuring this timer as a PWM (though this could be split off if
>> necessary). This whole driver lives in clocksource because it is primarily
>> clocksource stuff now (even though it started out as a PWM driver). I think
>> teasing apart the driver would not be worth it since they share so many
>> functions.
>>
>> This driver configures timer 0 (which is always present) as a clocksource,
>> and timer 1 (which might be missing) as a clockevent. I don't know if this
>> is the correct priority for these timers, or whether we should be using a
>> more dynamic allocation scheme.
>>
>> At the moment clock control is very basic: we just enable the clock during
>> probe and pin the frequency. In the future, someone could add support for
>> disabling the clock when not in use. Cascade mode is also unsupported.
>>
>> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
>>
>> [1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
>>
>> Signed-off-by: Sean Anderson [off-list ref]
>> ---
>> Please let me know if I should organize this differently or if it should
>> be broken up.
>>
>> Changes in v3:
>> - Add clockevent and clocksource support
>> - Rewrite probe to only use a device_node, since timers may need to be
>> initialized before we have proper devices. This does bloat the code a bit
>> since we can no longer rely on helpers such as dev_err_probe. We also
>> cannot rely on device resources being free'd on failure, so we must free
>> them manually.
>> - We now access registers through xilinx_timer_(read|write). This allows us
>> to deal with endianness issues, as originally seen in the microblaze
>> driver. CAVEAT EMPTOR: I have not tested this on big-endian!
>> - Remove old microblaze driver
>>
>> Changes in v2:
>> - Don't compile this module by default for arm64
>> - Add dependencies on COMMON_CLK and HAS_IOMEM
>> - Add comment explaining why we depend on !MICROBLAZE
>> - Add comment describing device
>> - Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
>> - Use NSEC_TO_SEC instead of defining our own
>> - Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by Uwe
>> - Cast dividends to u64 to avoid overflow
>> - Check for over- and underflow when calculating TLR
>> - Set xilinx_pwm_ops.owner
>> - Don't set pwmchip.base to -1
>> - Check range of xlnx,count-width
>> - Ensure the clock is always running when the pwm is registered
>> - Remove debugfs file :l
>> - Report errors with dev_error_probe
>>
>> arch/microblaze/kernel/Makefile | 2 +-
>> arch/microblaze/kernel/timer.c | 326 ---------------
>> drivers/clocksource/Kconfig | 15 +
>> drivers/clocksource/Makefile | 1 +
>> drivers/clocksource/timer-xilinx.c | 650 +++++++++++++++++++++++++++++
>> 5 files changed, 667 insertions(+), 327 deletions(-)
>> delete mode 100644 arch/microblaze/kernel/timer.c
>> create mode 100644 drivers/clocksource/timer-xilinx.c
>
> I don't think this is the right way to go.
> The first patch should be move current timer driver from microblaze to
> generic location and then apply patches on the top based on what you are
> adding/fixing to be able to review every change separately.
> When any issue happens it can be bisected and exact patch is identified.
> With this way we will end up in this patch and it will take a lot of
> time to find where that problem is.
What parts would you like to see split? Fundamentally, this current
patch is a reimplementation of the driver. I think the only reasonable
split would be to add PWM support in a separate patch.
I do not think that genericizing the microblaze timer driver is an
integral part of adding PWM support. This is especially since you seem
opposed to using existing devicetree properties to inform the driver. I
am inclined to just add a patch adding a check for '#-pwm-cells' to the
existing driver and otherwise leave it untouched.
> Another part of this is that you have c&p some parts from origin driver
> and do not keep origin authors there which can be consider as license
> violation.
I have not copy-pasted any code from the original driver. All of this
was written by consulting with the datasheet and other timer drivers in
the Linux kernel. In some instances I have referred to the original
driver (such as when discovering the need for detecting endianness) but
none of the original code was re-used. As it happens, since these
drivers are accomplishing the same task, some code is necessarily going
to be similar. Therefore, I have not added the copyright lines from the
original driver.
--Sean
>
> Thanks,
> Michal
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Sean Anderson <hidden> Date: 2021-05-14 17:13:45
On 5/14/21 4:50 AM, Michal Simek wrote:
>
>
> On 5/13/21 10:43 PM, Rob Herring wrote:
>> On Thu, May 13, 2021 at 10:28 AM Sean Anderson [off-list ref] wrote:
>>>
>>>
>>>
>>> On 5/13/21 10:33 AM, Sean Anderson wrote:
>>> >
>>> >
>>> > On 5/12/21 10:16 PM, Rob Herring wrote:
>>> > > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
>>> > >> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
>>> > >> a "soft" block, so it has many parameters which would not be
>>> > >> configurable in most hardware. This binding is usually automatically
>>> > >> generated by Xilinx's tools, so the names and values of some properties
>>> > >> must be kept as they are. Replacement properties have been provided for
>>> > >> new device trees.
>>> > >
>>> > > Because you have some tool generating properties is not a reason we have
>>> > > to accept them upstream.
>>> >
>>> > These properties are already in arch/microblaze/boot/dts/system.dts and
>>> > in the devicetree supplied to Linux by qemu. Removing these properties
>>> > will break existing setups, which I would like to avoid.
>>
>> Already in use in upstream dts files is different than just
>> 'automatically generated' by vendor tools.
>>
>>> >
>>> > > 'deprecated' is for what *we* have deprecated.
>>> >
>>> > Ok. I will remove that then.
>>> >
>>> > >
>>> > > In this case, I don't really see the point in defining new properties
>>> > > just to have bool.
>>> >
>>> > I don't either, but it was requested, by Michal...
>>>
>>> Err, your comment on the original bindings was
>>>
>>> > Can't all these be boolean?
>>
>> With no other context, yes that's what I would ask. Now you've given
>> me some context, between using the existing ones and 2 sets of
>> properties to maintain, I choose the former.
>>
>>> And Michal commented
>>>
>>> > I think in this case you should described what it is used by current
>>> > driver in Microblaze and these options are required. The rest are by
>>> > design optional.
>>> > If you want to change them to different value then current binding
>>> > should be deprecated and have any transition time with code alignment.
>>>
>>> So that is what I tried to accomplish with this revision. I also tried
>>> allowing something like
>>>
>>> xlnx,one-timer-only = <0>; /* two timers */
>>> xlnx,one-timer-only = <1>; /* one timer */
>>> xlnx,one-timer-only; /* one timer */
>>> /* property absent means two timers */
>>>
>>> but I was unable to figure out how to express this with json-schema. I
>>> don't think it's the best design either...
>>
>> json-schema would certainly let you, but generally we don't want
>> properties to have more than 1 type.
>
> One thing is what it is in system.dts file which was committed in 2009
> and there are just small alignments there. But none is really using it.
> Maybe I should just delete it.
> And this version was generated by Xilinx ancient tools at that time. All
> parameters there are fully describing HW and they are not changing. Only
> new one can be added.
>
> From the current microblaze code you can see which properties are really
> used.
>
> reg
> interrupts
> xlnx,one-timer-only
> clocks
> clock-frequency
There is also an implicit dependency on xlnx,count-width. Several times
the existing driver assumes the counter width is 32, but this should
instead be discovered from the devicetree.
> It means from my point of view these should be listed in the binding.
> clock-frequency is optional by code when clock is defined.
>
> All other properties listed in system.dts are from my perspective
> optional and that's how it should be.
Here is the situation as I understand it
* This device has existed for around 15 years (since 2006)
* Because it is a soft device, there are several configurable parameters
* Although all of these parameters must be known for a complete
implementation of this device, some are unnecessary if onlu reduced
functionality is needed.
* A de facto devicetree binding for this device has existed for at least
12 years (since 2009), but likely for as long as the device itself has
existed. This binding has not changed substantially during this time.
* This binding is present in devicetrees from the Linux kernel, from
qemu, in other existing systems, and in devicetrees generated by
Xilinx's toolset.
* Because the existing driver for this device does not implement all
functionality for this device, not all properties in the devicetree
binding are used. In fact, there is (as noted above) one property
which should be in use but is not because the current driver
(implicitly) does not support some hardware configurations.
* To support additional functionality, it is necessary to
use hardware parameters which were not previously necessary.
Based on the above, we can classify the properties of this binding into
several categories.
* Those which are currently read by the driver.
* compatible
* reg
* clocks
* clock-frequency
* interrupts
* xlnx,one-timer-only
* Those which reflect hardware parameters which are currently explicitly
or implicitly relied upon by the driver.
* reg
* clocks
* clock-frequency
* interrupts
* xlnx,counter-width
* xlnx,one-timer-only
* Those which are currently present in device trees.
* compatible
* reg
* interrupts
* clocks
* clock-frequency
* xlnx,count-width
* xlnx,one-timer-only
* xlnx,trig0-assert
* xlnx,trig1-assert
* xlnx,gen0-assert
* xlnx,gen1-assert
When choosing what properties to use, we must consider what the impact
of our changes will be on not just the kernel but also on existing users
of this binding:
* To use properties currently present in device trees, we just need to
modify the kernel driver.
* To add additional properties (such as e.g. '#pwm-cells'), we must
modify the kernel driver. In addition, users who would like to use
these new properties must add them to their device trees. This may be
done in a mechanical way using e.g. overlays.
* To deprecate existing properties and introduce new properties to
expose the same underlying hardware parameters, we must modify the
kernel driver. However, this has a large impact on existing users.
They must modify their tools to generate this information in a
different format. When this information is generated by upstream tools
this may require updating a core part of their build system. For many
projects, this may happen very infrequently because of the risk that
such an upgrade will break things. Even if you suggest that Xilinx can
easily modify its tools to generate any sort of output, the time for
this upgrade to be deployed/adopted may be significantly longer.
Note that while all three types of changes are similar from a kernel
point of view, the impact on existing users is much large in the latter
case. For this reason, I think that wherever possible we should use
properties which are already present in existing device trees.
> I think DT binding patch should reflect this state as patch itself.
> And then PWM should be added on the top as separate patch.
I have no preference here.
--Sean
>
> Note: In past we were using only parameters and name we got from tools
> but over years we were fine to use for example bool properties and we
> just aligned Xilinx device tree generator to match it. That's why not a
> problem to deprecate any property and move to new one. Xilinx DTG is
> already prepared for it and it is easy to remap it.
>
> Thanks,
> Michal
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Signed-off-by: Sean Anderson <redacted>
---
Please let me know if I should organize this differently or if it should
be broken up.
Changes in v3:
- Add clockevent and clocksource support
- Rewrite probe to only use a device_node, since timers may need to be
initialized before we have proper devices. This does bloat the
code a bit
quoted
quoted
since we can no longer rely on helpers such as dev_err_probe. We also
cannot rely on device resources being free'd on failure, so we
must free
quoted
quoted
them manually.
- We now access registers through xilinx_timer_(read|write). This
allows us
quoted
quoted
to deal with endianness issues, as originally seen in the microblaze
driver. CAVEAT EMPTOR: I have not tested this on big-endian!
- Remove old microblaze driver
Changes in v2:
- Don't compile this module by default for arm64
- Add dependencies on COMMON_CLK and HAS_IOMEM
- Add comment explaining why we depend on !MICROBLAZE
- Add comment describing device
- Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
- Use NSEC_TO_SEC instead of defining our own
- Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by Uwe
- Cast dividends to u64 to avoid overflow
- Check for over- and underflow when calculating TLR
- Set xilinx_pwm_ops.owner
- Don't set pwmchip.base to -1
- Check range of xlnx,count-width
- Ensure the clock is always running when the pwm is registered
- Remove debugfs file :l
- Report errors with dev_error_probe
arch/microblaze/kernel/Makefile | 2 +-
arch/microblaze/kernel/timer.c | 326 ---------------
drivers/clocksource/Kconfig | 15 +
drivers/clocksource/Makefile | 1 +
drivers/clocksource/timer-xilinx.c | 650 +++++++++++++++++++++++++++++
5 files changed, 667 insertions(+), 327 deletions(-)
delete mode 100644 arch/microblaze/kernel/timer.c
create mode 100644 drivers/clocksource/timer-xilinx.c
I don't think this is the right way to go.
The first patch should be move current timer driver from microblaze to
generic location and then apply patches on the top based on what you are
adding/fixing to be able to review every change separately.
When any issue happens it can be bisected and exact patch is identified.
With this way we will end up in this patch and it will take a lot of
time to find where that problem is.
What parts would you like to see split? Fundamentally, this current
patch is a reimplementation of the driver. I think the only reasonable
split would be to add PWM support in a separate patch.
I do not think that genericizing the microblaze timer driver is an
integral part of adding PWM support. This is especially since you seem
opposed to using existing devicetree properties to inform the driver. I
am inclined to just add a patch adding a check for '#-pwm-cells' to the
existing driver and otherwise leave it untouched.
As I said I think the patches should be like this.
1. Cover existing DT binding based on current code.
2. Move time out of arch/microblaze to drivers/clocksource/ and even
enable it via Kconfig just for Microblaze.
3. Remove dependency on Microblaze and enable build for others. I have
seen at least one cpuinfo.cpu_clock_freq assignment. This code can be
likely completely removed or deprecate.
4. Make driver as module
5. Do whatever changes you want before adding pwm support
6. Extend DT binding doc for PWM support
7. Add PWM support
I expect you know that some time ago we have also added support for
Microblaze SMP and this code has never been sent upstream. You should
just be aware about it.
https://github.com/Xilinx/linux-xlnx/blob/master/arch/microblaze/kernel/timer.c
Thanks,
Michal
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Michal Simek <hidden> Date: 2021-05-17 08:28:26
On 5/14/21 7:13 PM, Sean Anderson wrote:
On 5/14/21 4:50 AM, Michal Simek wrote:
quoted
On 5/13/21 10:43 PM, Rob Herring wrote:
quoted
On Thu, May 13, 2021 at 10:28 AM Sean Anderson
[off-list ref] wrote:
quoted
quoted
quoted
On 5/13/21 10:33 AM, Sean Anderson wrote:
>
>
> On 5/12/21 10:16 PM, Rob Herring wrote:
> > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
> >> This adds a binding for the Xilinx LogiCORE IP AXI Timer.
This device is
quoted
quoted
quoted
> >> a "soft" block, so it has many parameters which would not be
> >> configurable in most hardware. This binding is usually
automatically
quoted
quoted
quoted
> >> generated by Xilinx's tools, so the names and values of some
properties
quoted
quoted
quoted
> >> must be kept as they are. Replacement properties have been
provided for
quoted
quoted
quoted
> >> new device trees.
> >
> > Because you have some tool generating properties is not a
reason we have
quoted
quoted
quoted
> > to accept them upstream.
>
> These properties are already in
arch/microblaze/boot/dts/system.dts and
quoted
quoted
quoted
> in the devicetree supplied to Linux by qemu. Removing these
properties
quoted
quoted
quoted
> will break existing setups, which I would like to avoid.
Already in use in upstream dts files is different than just
'automatically generated' by vendor tools.
quoted
>
> > 'deprecated' is for what *we* have deprecated.
>
> Ok. I will remove that then.
>
> >
> > In this case, I don't really see the point in defining new
properties
quoted
quoted
quoted
> > just to have bool.
>
> I don't either, but it was requested, by Michal...
Err, your comment on the original bindings was
> Can't all these be boolean?
With no other context, yes that's what I would ask. Now you've given
me some context, between using the existing ones and 2 sets of
properties to maintain, I choose the former.
quoted
And Michal commented
> I think in this case you should described what it is used by
current
quoted
quoted
quoted
> driver in Microblaze and these options are required. The rest
are by
quoted
quoted
quoted
> design optional.
> If you want to change them to different value then current binding
> should be deprecated and have any transition time with code
alignment.
quoted
quoted
quoted
So that is what I tried to accomplish with this revision. I also tried
allowing something like
xlnx,one-timer-only = <0>; /* two timers */
xlnx,one-timer-only = <1>; /* one timer */
xlnx,one-timer-only; /* one timer */
/* property absent means two timers */
but I was unable to figure out how to express this with json-schema. I
don't think it's the best design either...
json-schema would certainly let you, but generally we don't want
properties to have more than 1 type.
One thing is what it is in system.dts file which was committed in 2009
and there are just small alignments there. But none is really using it.
Maybe I should just delete it.
And this version was generated by Xilinx ancient tools at that time. All
parameters there are fully describing HW and they are not changing. Only
new one can be added.
From the current microblaze code you can see which properties are really
used.
reg
interrupts
xlnx,one-timer-only
clocks
clock-frequency
There is also an implicit dependency on xlnx,count-width. Several times
the existing driver assumes the counter width is 32, but this should
instead be discovered from the devicetree.
For me it is important what it is used now. Which is not
xlnx,count-width. That's why if you want to add it you can as optional
property.
quoted
It means from my point of view these should be listed in the binding.
clock-frequency is optional by code when clock is defined.
All other properties listed in system.dts are from my perspective
optional and that's how it should be.
Here is the situation as I understand it
* This device has existed for around 15 years (since 2006)
* Because it is a soft device, there are several configurable parameters
* Although all of these parameters must be known for a complete
implementation of this device, some are unnecessary if onlu reduced
functionality is needed.
* A de facto devicetree binding for this device has existed for at least
12 years (since 2009), but likely for as long as the device itself has
existed. This binding has not changed substantially during this time.
note: IP itself is even much older.
* This binding is present in devicetrees from the Linux kernel, from
qemu, in other existing systems, and in devicetrees generated by
Xilinx's toolset.
Only from Linux. Qemu is trying to reuse the same properties but it can
also add own one. They are trying to be aligned as much as possible but
there are a lot of cases where Qemu requires much more information. (I
am not saying in this timer case but in general).
* Because the existing driver for this device does not implement all
functionality for this device, not all properties in the devicetree
binding are used. In fact, there is (as noted above) one property
which should be in use but is not because the current driver
(implicitly) does not support some hardware configurations.
* To support additional functionality, it is necessary to
use hardware parameters which were not previously necessary.
Based on the above, we can classify the properties of this binding into
several categories.
* Those which are currently read by the driver.
* compatible
* reg
* clocks
* clock-frequency
* interrupts
* xlnx,one-timer-only
* Those which reflect hardware parameters which are currently explicitly
or implicitly relied upon by the driver.
* reg
* clocks
* clock-frequency
* interrupts
* xlnx,counter-width
* xlnx,one-timer-only
* Those which are currently present in device trees.
* compatible
* reg
* interrupts
* clocks
* clock-frequency
* xlnx,count-width
* xlnx,one-timer-only
* xlnx,trig0-assert
* xlnx,trig1-assert
* xlnx,gen0-assert
* xlnx,gen1-assert
When choosing what properties to use, we must consider what the impact
of our changes will be on not just the kernel but also on existing users
of this binding:
I don't think that this is valid. Rob is asking for adding #pwm-cells
which is purely Linux binding. We also don't know what properties are
used by others projects not just Linux or Qemu. Also required properties
in Linux doesn't need to be required in U-Boot for example even we are
trying to aligned all of them. Another case are others RTOSes, etc.
* To use properties currently present in device trees, we just need to
modify the kernel driver.
* To add additional properties (such as e.g. '#pwm-cells'), we must
modify the kernel driver. In addition, users who would like to use
these new properties must add them to their device trees. This may be
done in a mechanical way using e.g. overlays.
* To deprecate existing properties and introduce new properties to
expose the same underlying hardware parameters, we must modify the
kernel driver. However, this has a large impact on existing users.
They must modify their tools to generate this information in a
different format. When this information is generated by upstream tools
this may require updating a core part of their build system. For many
projects, this may happen very infrequently because of the risk that
such an upgrade will break things. Even if you suggest that Xilinx can
easily modify its tools to generate any sort of output, the time for
this upgrade to be deployed/adopted may be significantly longer.
From Xilinx perspective it would be ideal to use only properties which
fully describe HW in the form how they are generated today. They are
stable for a lot of years and as I said only new one are added.
But this alignment wasn't accepted long time ago and we have been asked
to start to align these properties with similar HW done by others.
And truth is that in a lot of cases there is clear 1:1 mapping and
generic properties can be simply use. This mapping ends in Xilinx device
tree generator.
Back to your point. Required properties are required by Linux driver
only. This driver is around for quite a long time where certain policies
haven't been setup/used/enforced (Microblaze is 2nd architecture which
started to use device tree).
We should create DT binding doc at that time but in 2009 it wasn't
standard practice. In 2007 Grant was adding support for Xilinx PPC
platform also without any DT binding document too.
That's why we need to review current unwritten DT binding based on code
requirements and look at it how to fix it (if needed) and then add PWM
support on the top of it.
If something needs to be deprecated, let's deprecate it and have
transition time for a year or so to adapt to it.
Rob knows much better than I how this should be handled.
Based on your list:
* compatible
* reg
* clocks
* clock-frequency
* interrupts
* xlnx,one-timer-only
all of these are required property in new DT binding.
xlnx,counter-width is optional if you want to use.
#pwm-cells is optional for enabling PWM support (I would expect that
when this property is present this timer don't need be used as
clocksource/clockevent by Microblaze)
etc
And for properties which are generated out of Xilinx tools I would allow
in DT binding to add others optional properties that DT won't error out
if they are seen.
Thanks,
Michal
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Sean Anderson <hidden> Date: 2021-05-17 14:48:49
On 5/17/21 4:28 AM, Michal Simek wrote:
>
>
> On 5/14/21 7:13 PM, Sean Anderson wrote:
>>
>>
>> On 5/14/21 4:50 AM, Michal Simek wrote:
>>>
>>>
>>> On 5/13/21 10:43 PM, Rob Herring wrote:
>>>> On Thu, May 13, 2021 at 10:28 AM Sean Anderson
>> [off-list ref] wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 5/13/21 10:33 AM, Sean Anderson wrote:
>>>>> >
>>>>> >
>>>>> > On 5/12/21 10:16 PM, Rob Herring wrote:
>>>>> > > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
>>>>> > >> This adds a binding for the Xilinx LogiCORE IP AXI Timer.
>> This device is
>>>>> > >> a "soft" block, so it has many parameters which would not be
>>>>> > >> configurable in most hardware. This binding is usually
>> automatically
>>>>> > >> generated by Xilinx's tools, so the names and values of some
>> properties
>>>>> > >> must be kept as they are. Replacement properties have been
>> provided for
>>>>> > >> new device trees.
>>>>> > >
>>>>> > > Because you have some tool generating properties is not a
>> reason we have
>>>>> > > to accept them upstream.
>>>>> >
>>>>> > These properties are already in
>> arch/microblaze/boot/dts/system.dts and
>>>>> > in the devicetree supplied to Linux by qemu. Removing these
>> properties
>>>>> > will break existing setups, which I would like to avoid.
>>>>
>>>> Already in use in upstream dts files is different than just
>>>> 'automatically generated' by vendor tools.
>>>>
>>>>> >
>>>>> > > 'deprecated' is for what *we* have deprecated.
>>>>> >
>>>>> > Ok. I will remove that then.
>>>>> >
>>>>> > >
>>>>> > > In this case, I don't really see the point in defining new
>> properties
>>>>> > > just to have bool.
>>>>> >
>>>>> > I don't either, but it was requested, by Michal...
>>>>>
>>>>> Err, your comment on the original bindings was
>>>>>
>>>>> > Can't all these be boolean?
>>>>
>>>> With no other context, yes that's what I would ask. Now you've given
>>>> me some context, between using the existing ones and 2 sets of
>>>> properties to maintain, I choose the former.
>>>>
>>>>> And Michal commented
>>>>>
>>>>> > I think in this case you should described what it is used by
>> current
>>>>> > driver in Microblaze and these options are required. The rest
>> are by
>>>>> > design optional.
>>>>> > If you want to change them to different value then current binding
>>>>> > should be deprecated and have any transition time with code
>> alignment.
>>>>>
>>>>> So that is what I tried to accomplish with this revision. I also tried
>>>>> allowing something like
>>>>>
>>>>> xlnx,one-timer-only = <0>; /* two timers */
>>>>> xlnx,one-timer-only = <1>; /* one timer */
>>>>> xlnx,one-timer-only; /* one timer */
>>>>> /* property absent means two timers */
>>>>>
>>>>> but I was unable to figure out how to express this with json-schema. I
>>>>> don't think it's the best design either...
>>>>
>>>> json-schema would certainly let you, but generally we don't want
>>>> properties to have more than 1 type.
>>>
>>> One thing is what it is in system.dts file which was committed in 2009
>>> and there are just small alignments there. But none is really using it.
>>> Maybe I should just delete it.
>>> And this version was generated by Xilinx ancient tools at that time. All
>>> parameters there are fully describing HW and they are not changing. Only
>>> new one can be added.
>>>
>>> From the current microblaze code you can see which properties are really
>>> used.
>>>
>>> reg
>>> interrupts
>>> xlnx,one-timer-only
>>> clocks
>>> clock-frequency
>>
>> There is also an implicit dependency on xlnx,count-width. Several times
>> the existing driver assumes the counter width is 32, but this should
>> instead be discovered from the devicetree.
>
> For me it is important what it is used now. Which is not
> xlnx,count-width. That's why if you want to add it you can as optional
> property.
At the very least we should sanity check it. E.g. check that it is 32
and return -EINVAL if it is not.
>
>>
>>> It means from my point of view these should be listed in the binding.
>>> clock-frequency is optional by code when clock is defined.
>>>
>>> All other properties listed in system.dts are from my perspective
>>> optional and that's how it should be.
>>
>> Here is the situation as I understand it
>>
>> * This device has existed for around 15 years (since 2006)
>> * Because it is a soft device, there are several configurable parameters
>> * Although all of these parameters must be known for a complete
>> implementation of this device, some are unnecessary if onlu reduced
>> functionality is needed.
>> * A de facto devicetree binding for this device has existed for at least
>> 12 years (since 2009), but likely for as long as the device itself has
>> existed. This binding has not changed substantially during this time.
>
> note: IP itself is even much older.
>
>> * This binding is present in devicetrees from the Linux kernel, from
>> qemu, in other existing systems, and in devicetrees generated by
>> Xilinx's toolset.
>
> Only from Linux. Qemu is trying to reuse the same properties but it can
> also add own one. They are trying to be aligned as much as possible but
> there are a lot of cases where Qemu requires much more information. (I
> am not saying in this timer case but in general).
>
>
>> * Because the existing driver for this device does not implement all
>> functionality for this device, not all properties in the devicetree
>> binding are used. In fact, there is (as noted above) one property
>> which should be in use but is not because the current driver
>> (implicitly) does not support some hardware configurations.
>> * To support additional functionality, it is necessary to
>> use hardware parameters which were not previously necessary.
>>
>> Based on the above, we can classify the properties of this binding into
>> several categories.
>>
>> * Those which are currently read by the driver.
>> * compatible
>> * reg
>> * clocks
>> * clock-frequency
>> * interrupts
>> * xlnx,one-timer-only
>>
>> * Those which reflect hardware parameters which are currently explicitly
>> or implicitly relied upon by the driver.
>> * reg
>> * clocks
>> * clock-frequency
>> * interrupts
>> * xlnx,counter-width
>> * xlnx,one-timer-only
>>
>> * Those which are currently present in device trees.
>> * compatible
>> * reg
>> * interrupts
>> * clocks
>> * clock-frequency
>> * xlnx,count-width
>> * xlnx,one-timer-only
>> * xlnx,trig0-assert
>> * xlnx,trig1-assert
>> * xlnx,gen0-assert
>> * xlnx,gen1-assert
>>
>> When choosing what properties to use, we must consider what the impact
>> of our changes will be on not just the kernel but also on existing users
>> of this binding:
>
> I don't think that this is valid. Rob is asking for adding #pwm-cells
> which is purely Linux binding. We also don't know what properties are
> used by others projects not just Linux or Qemu. Also required properties
> in Linux doesn't need to be required in U-Boot for example even we are
> trying to aligned all of them. Another case are others RTOSes, etc.
Here I do not see a way around this. Any way we do it we will need to
have some new binding. However, as noted below, adding a new binding for
configuration is easier than exposing properties in new ways.
>> * To use properties currently present in device trees, we just need to
>> modify the kernel driver.
>> * To add additional properties (such as e.g. '#pwm-cells'), we must
>> modify the kernel driver. In addition, users who would like to use
>> these new properties must add them to their device trees. This may be
>> done in a mechanical way using e.g. overlays.
>> * To deprecate existing properties and introduce new properties to
>> expose the same underlying hardware parameters, we must modify the
>> kernel driver. However, this has a large impact on existing users.
>> They must modify their tools to generate this information in a
>> different format. When this information is generated by upstream tools
>> this may require updating a core part of their build system. For many
>> projects, this may happen very infrequently because of the risk that
>> such an upgrade will break things. Even if you suggest that Xilinx can
>> easily modify its tools to generate any sort of output, the time for
>> this upgrade to be deployed/adopted may be significantly longer.
>
> From Xilinx perspective it would be ideal to use only properties which
> fully describe HW in the form how they are generated today. They are
> stable for a lot of years and as I said only new one are added.
> But this alignment wasn't accepted long time ago and we have been asked
> to start to align these properties with similar HW done by others.
> And truth is that in a lot of cases there is clear 1:1 mapping and
> generic properties can be simply use. This mapping ends in Xilinx device
> tree generator.
> Back to your point. Required properties are required by Linux driver
> only. This driver is around for quite a long time where certain policies
> haven't been setup/used/enforced (Microblaze is 2nd architecture which
> started to use device tree).
> We should create DT binding doc at that time but in 2009 it wasn't
> standard practice. In 2007 Grant was adding support for Xilinx PPC
> platform also without any DT binding document too.
>
> That's why we need to review current unwritten DT binding based on code
> requirements and look at it how to fix it (if needed) and then add PWM
> support on the top of it.
> If something needs to be deprecated, let's deprecate it and have
> transition time for a year or so to adapt to it.
>
> Rob knows much better than I how this should be handled.
>
> Based on your list:
> * compatible
> * reg
> * clocks
> * clock-frequency
> * interrupts
> * xlnx,one-timer-only
>
> all of these are required property in new DT binding.
>
> xlnx,counter-width is optional if you want to use.
Again, for the existing driver this should at least be sanity-checked so
we fail noisily instead of buggily.
This is the situation for the generate polarity as well. The driver will
work fine, but you will not have PWM output and there will be no
indication why. This is why I think we should start with supporting the
existing output. This is used to ensure the device will work as
configured. We can also add support for different properties exposing
the same information, but to support only new properties is not very
useful.
--Sean
> #pwm-cells is optional for enabling PWM support (I would expect that
> when this property is present this timer don't need be used as
> clocksource/clockevent by Microblaze)
> etc
>
> And for properties which are generated out of Xilinx tools I would allow
> in DT binding to add others optional properties that DT won't error out
> if they are seen.
>
> Thanks,
> Michal
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Michal Simek <hidden> Date: 2021-05-17 14:59:59
On 5/17/21 4:40 PM, Sean Anderson wrote:
On 5/17/21 4:28 AM, Michal Simek wrote:
quoted
On 5/14/21 7:13 PM, Sean Anderson wrote:
quoted
On 5/14/21 4:50 AM, Michal Simek wrote:
quoted
On 5/13/21 10:43 PM, Rob Herring wrote:
quoted
On Thu, May 13, 2021 at 10:28 AM Sean Anderson
[off-list ref] wrote:
quoted
quoted
quoted
On 5/13/21 10:33 AM, Sean Anderson wrote:
>
>
> On 5/12/21 10:16 PM, Rob Herring wrote:
> > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson
wrote:
quoted
quoted
quoted
quoted
quoted
> >> This adds a binding for the Xilinx LogiCORE IP AXI Timer.
This device is
quoted
quoted
quoted
> >> a "soft" block, so it has many parameters which would
not be
quoted
quoted
quoted
quoted
quoted
> >> configurable in most hardware. This binding is usually
automatically
quoted
quoted
quoted
> >> generated by Xilinx's tools, so the names and values of
some
quoted
quoted
properties
quoted
quoted
quoted
> >> must be kept as they are. Replacement properties have been
provided for
quoted
quoted
quoted
> >> new device trees.
> >
> > Because you have some tool generating properties is not a
reason we have
quoted
quoted
quoted
> > to accept them upstream.
>
> These properties are already in
arch/microblaze/boot/dts/system.dts and
quoted
quoted
quoted
> in the devicetree supplied to Linux by qemu. Removing these
properties
quoted
quoted
quoted
> will break existing setups, which I would like to avoid.
Already in use in upstream dts files is different than just
'automatically generated' by vendor tools.
quoted
>
> > 'deprecated' is for what *we* have deprecated.
>
> Ok. I will remove that then.
>
> >
> > In this case, I don't really see the point in defining new
properties
quoted
quoted
quoted
> > just to have bool.
>
> I don't either, but it was requested, by Michal...
Err, your comment on the original bindings was
> Can't all these be boolean?
With no other context, yes that's what I would ask. Now you've given
me some context, between using the existing ones and 2 sets of
properties to maintain, I choose the former.
quoted
And Michal commented
> I think in this case you should described what it is used by
current
quoted
quoted
quoted
> driver in Microblaze and these options are required. The rest
are by
quoted
quoted
quoted
> design optional.
> If you want to change them to different value then current
binding
quoted
quoted
quoted
quoted
quoted
> should be deprecated and have any transition time with code
alignment.
quoted
quoted
quoted
So that is what I tried to accomplish with this revision. I also
tried
quoted
quoted
quoted
quoted
quoted
allowing something like
xlnx,one-timer-only = <0>; /* two timers */
xlnx,one-timer-only = <1>; /* one timer */
xlnx,one-timer-only; /* one timer */
/* property absent means two timers */
but I was unable to figure out how to express this with
json-schema. I
quoted
quoted
quoted
quoted
quoted
don't think it's the best design either...
json-schema would certainly let you, but generally we don't want
properties to have more than 1 type.
One thing is what it is in system.dts file which was committed in 2009
and there are just small alignments there. But none is really using it.
Maybe I should just delete it.
And this version was generated by Xilinx ancient tools at that time.
All
quoted
quoted
quoted
parameters there are fully describing HW and they are not changing.
Only
quoted
quoted
quoted
new one can be added.
From the current microblaze code you can see which properties are
There is also an implicit dependency on xlnx,count-width. Several times
the existing driver assumes the counter width is 32, but this should
instead be discovered from the devicetree.
For me it is important what it is used now. Which is not
xlnx,count-width. That's why if you want to add it you can as optional
property.
At the very least we should sanity check it. E.g. check that it is 32
and return -EINVAL if it is not.
I have not a problem with it but make sure that the check is there only
when property is present not to break all current users.
quoted
quoted
quoted
It means from my point of view these should be listed in the binding.
clock-frequency is optional by code when clock is defined.
All other properties listed in system.dts are from my perspective
optional and that's how it should be.
Here is the situation as I understand it
* This device has existed for around 15 years (since 2006)
* Because it is a soft device, there are several configurable parameters
* Although all of these parameters must be known for a complete
implementation of this device, some are unnecessary if onlu reduced
functionality is needed.
* A de facto devicetree binding for this device has existed for at least
12 years (since 2009), but likely for as long as the device itself
has
quoted
quoted
existed. This binding has not changed substantially during this time.
note: IP itself is even much older.
quoted
* This binding is present in devicetrees from the Linux kernel, from
qemu, in other existing systems, and in devicetrees generated by
Xilinx's toolset.
Only from Linux. Qemu is trying to reuse the same properties but it can
also add own one. They are trying to be aligned as much as possible but
there are a lot of cases where Qemu requires much more information. (I
am not saying in this timer case but in general).
quoted
* Because the existing driver for this device does not implement all
functionality for this device, not all properties in the devicetree
binding are used. In fact, there is (as noted above) one property
which should be in use but is not because the current driver
(implicitly) does not support some hardware configurations.
* To support additional functionality, it is necessary to
use hardware parameters which were not previously necessary.
Based on the above, we can classify the properties of this binding into
several categories.
* Those which are currently read by the driver.
* compatible
* reg
* clocks
* clock-frequency
* interrupts
* xlnx,one-timer-only
* Those which reflect hardware parameters which are currently explicitly
or implicitly relied upon by the driver.
* reg
* clocks
* clock-frequency
* interrupts
* xlnx,counter-width
* xlnx,one-timer-only
* Those which are currently present in device trees.
* compatible
* reg
* interrupts
* clocks
* clock-frequency
* xlnx,count-width
* xlnx,one-timer-only
* xlnx,trig0-assert
* xlnx,trig1-assert
* xlnx,gen0-assert
* xlnx,gen1-assert
When choosing what properties to use, we must consider what the impact
of our changes will be on not just the kernel but also on existing users
of this binding:
I don't think that this is valid. Rob is asking for adding #pwm-cells
which is purely Linux binding. We also don't know what properties are
used by others projects not just Linux or Qemu. Also required properties
in Linux doesn't need to be required in U-Boot for example even we are
trying to aligned all of them. Another case are others RTOSes, etc.
Here I do not see a way around this. Any way we do it we will need to
have some new binding. However, as noted below, adding a new binding for
configuration is easier than exposing properties in new ways.
please look below.
quoted
quoted
* To use properties currently present in device trees, we just need to
modify the kernel driver.
* To add additional properties (such as e.g. '#pwm-cells'), we must
modify the kernel driver. In addition, users who would like to use
these new properties must add them to their device trees. This may be
done in a mechanical way using e.g. overlays.
* To deprecate existing properties and introduce new properties to
expose the same underlying hardware parameters, we must modify the
kernel driver. However, this has a large impact on existing users.
They must modify their tools to generate this information in a
different format. When this information is generated by upstream
tools
quoted
quoted
this may require updating a core part of their build system. For many
projects, this may happen very infrequently because of the risk that
such an upgrade will break things. Even if you suggest that Xilinx
can
quoted
quoted
easily modify its tools to generate any sort of output, the time for
this upgrade to be deployed/adopted may be significantly longer.
From Xilinx perspective it would be ideal to use only properties which
fully describe HW in the form how they are generated today. They are
stable for a lot of years and as I said only new one are added.
But this alignment wasn't accepted long time ago and we have been asked
to start to align these properties with similar HW done by others.
And truth is that in a lot of cases there is clear 1:1 mapping and
generic properties can be simply use. This mapping ends in Xilinx device
tree generator.
Back to your point. Required properties are required by Linux driver
only. This driver is around for quite a long time where certain policies
haven't been setup/used/enforced (Microblaze is 2nd architecture which
started to use device tree).
We should create DT binding doc at that time but in 2009 it wasn't
standard practice. In 2007 Grant was adding support for Xilinx PPC
platform also without any DT binding document too.
That's why we need to review current unwritten DT binding based on code
requirements and look at it how to fix it (if needed) and then add PWM
support on the top of it.
If something needs to be deprecated, let's deprecate it and have
transition time for a year or so to adapt to it.
Rob knows much better than I how this should be handled.
Based on your list:
* compatible
* reg
* clocks
* clock-frequency
* interrupts
* xlnx,one-timer-only
all of these are required property in new DT binding.
xlnx,counter-width is optional if you want to use.
Again, for the existing driver this should at least be sanity-checked so
we fail noisily instead of buggily.
Till now for 10+ years none reported any issue with it that's why I
don't think this is a big problem.
This is the situation for the generate polarity as well. The driver will
work fine, but you will not have PWM output and there will be no
indication why. This is why I think we should start with supporting the
existing output. This is used to ensure the device will work as
configured. We can also add support for different properties exposing
the same information, but to support only new properties is not very
useful.
Binding can enforce properties to be required for PWM only and that's
totally okay. And because this is new feature and Rob don't need to like
that new properties they can be aligned. I also have preference to be as
much aligned with HW parameters but if they have to change then let's
change it and I will ensure that Xilinx DTG will generate them as they
were described to be aligned.
Thanks,
Michal
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Sean Anderson <hidden> Date: 2021-05-17 22:16:12
On 5/17/21 3:54 AM, Michal Simek wrote:
>
>
> On 5/14/21 4:40 PM, Sean Anderson wrote:
>>
>>
>> On 5/14/21 4:59 AM, Michal Simek wrote:
>>>
>>>
>>> On 5/11/21 9:12 PM, Sean Anderson wrote:
>>>> This adds generic clocksource and clockevent support for Xilinx
>> LogiCORE IP
>>>> AXI soft timers commonly found on Xilinx FPGAs. This timer is also the
>>>> primary timer for Microblaze processors. This commit also adds
>> support for
>>>> configuring this timer as a PWM (though this could be split off if
>>>> necessary). This whole driver lives in clocksource because it is
>> primarily
>>>> clocksource stuff now (even though it started out as a PWM driver). I
>> think
>>>> teasing apart the driver would not be worth it since they share so many
>>>> functions.
>>>>
>>>> This driver configures timer 0 (which is always present) as a
>> clocksource,
>>>> and timer 1 (which might be missing) as a clockevent. I don't know if
>> this
>>>> is the correct priority for these timers, or whether we should be
>> using a
>>>> more dynamic allocation scheme.
>>>>
>>>> At the moment clock control is very basic: we just enable the clock
>> during
>>>> probe and pin the frequency. In the future, someone could add support
>> for
>>>> disabling the clock when not in use. Cascade mode is also unsupported.
>>>>
>>>> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
>>>>
>>>> [1]
>> https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
>>
>>>>
>>>> Signed-off-by: Sean Anderson [off-list ref]
>>>> ---
>>>> Please let me know if I should organize this differently or if it should
>>>> be broken up.
>>>>
>>>> Changes in v3:
>>>> - Add clockevent and clocksource support
>>>> - Rewrite probe to only use a device_node, since timers may need to be
>>>> initialized before we have proper devices. This does bloat the
>> code a bit
>>>> since we can no longer rely on helpers such as dev_err_probe. We also
>>>> cannot rely on device resources being free'd on failure, so we
>> must free
>>>> them manually.
>>>> - We now access registers through xilinx_timer_(read|write). This
>> allows us
>>>> to deal with endianness issues, as originally seen in the microblaze
>>>> driver. CAVEAT EMPTOR: I have not tested this on big-endian!
>>>> - Remove old microblaze driver
>>>>
>>>> Changes in v2:
>>>> - Don't compile this module by default for arm64
>>>> - Add dependencies on COMMON_CLK and HAS_IOMEM
>>>> - Add comment explaining why we depend on !MICROBLAZE
>>>> - Add comment describing device
>>>> - Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
>>>> - Use NSEC_TO_SEC instead of defining our own
>>>> - Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by Uwe
>>>> - Cast dividends to u64 to avoid overflow
>>>> - Check for over- and underflow when calculating TLR
>>>> - Set xilinx_pwm_ops.owner
>>>> - Don't set pwmchip.base to -1
>>>> - Check range of xlnx,count-width
>>>> - Ensure the clock is always running when the pwm is registered
>>>> - Remove debugfs file :l
>>>> - Report errors with dev_error_probe
>>>>
>>>> arch/microblaze/kernel/Makefile | 2 +-
>>>> arch/microblaze/kernel/timer.c | 326 ---------------
>>>> drivers/clocksource/Kconfig | 15 +
>>>> drivers/clocksource/Makefile | 1 +
>>>> drivers/clocksource/timer-xilinx.c | 650 +++++++++++++++++++++++++++++
>>>> 5 files changed, 667 insertions(+), 327 deletions(-)
>>>> delete mode 100644 arch/microblaze/kernel/timer.c
>>>> create mode 100644 drivers/clocksource/timer-xilinx.c
>>>
>>> I don't think this is the right way to go.
>>> The first patch should be move current timer driver from microblaze to
>>> generic location and then apply patches on the top based on what you are
>>> adding/fixing to be able to review every change separately.
>>> When any issue happens it can be bisected and exact patch is identified.
>>> With this way we will end up in this patch and it will take a lot of
>>> time to find where that problem is.
>>
>> What parts would you like to see split? Fundamentally, this current
>> patch is a reimplementation of the driver. I think the only reasonable
>> split would be to add PWM support in a separate patch.
>>
>> I do not think that genericizing the microblaze timer driver is an
>> integral part of adding PWM support. This is especially since you seem
>> opposed to using existing devicetree properties to inform the driver. I
>> am inclined to just add a patch adding a check for '#-pwm-cells' to the
>> existing driver and otherwise leave it untouched.
>
> As I said I think the patches should be like this.
> 1. Cover existing DT binding based on current code.
> 2. Move time out of arch/microblaze to drivers/clocksource/ and even
> enable it via Kconfig just for Microblaze.
> 3. Remove dependency on Microblaze and enable build for others. I have
> seen at least one cpuinfo.cpu_clock_freq assignment. This code can be
> likely completely removed or deprecate.
This could be deprecated, but cannot be removed since existing device
trees (e.g. qemu) have neither clocks nor clock-frequency properties.
> 4. Make driver as module
> 5. Do whatever changes you want before adding pwm support
> 6. Extend DT binding doc for PWM support
> 7. Add PWM support
Frankly, I am inclined to just leave the microblaze timer as-is. The PWM
driver is completely independent. I have already put too much effort into
this driver, and I don't have the energy to continue working on the
microblaze timer.
--Sean
> I expect you know that some time ago we have also added support for
> Microblaze SMP and this code has never been sent upstream. You should
> just be aware about it.
> https://github.com/Xilinx/linux-xlnx/blob/master/arch/microblaze/kernel/timer.c
>
> Thanks,
> Michal
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Signed-off-by: Sean Anderson <redacted>
---
Please let me know if I should organize this differently or if it
should
quoted
quoted
quoted
quoted
be broken up.
Changes in v3:
- Add clockevent and clocksource support
- Rewrite probe to only use a device_node, since timers may need to be
initialized before we have proper devices. This does bloat the
code a bit
quoted
quoted
since we can no longer rely on helpers such as dev_err_probe.
We also
quoted
quoted
quoted
quoted
cannot rely on device resources being free'd on failure, so we
must free
quoted
quoted
them manually.
- We now access registers through xilinx_timer_(read|write). This
allows us
quoted
quoted
to deal with endianness issues, as originally seen in the
microblaze
quoted
quoted
quoted
quoted
driver. CAVEAT EMPTOR: I have not tested this on big-endian!
- Remove old microblaze driver
Changes in v2:
- Don't compile this module by default for arm64
- Add dependencies on COMMON_CLK and HAS_IOMEM
- Add comment explaining why we depend on !MICROBLAZE
- Add comment describing device
- Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
- Use NSEC_TO_SEC instead of defining our own
- Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by
Uwe
quoted
quoted
quoted
quoted
- Cast dividends to u64 to avoid overflow
- Check for over- and underflow when calculating TLR
- Set xilinx_pwm_ops.owner
- Don't set pwmchip.base to -1
- Check range of xlnx,count-width
- Ensure the clock is always running when the pwm is registered
- Remove debugfs file :l
- Report errors with dev_error_probe
arch/microblaze/kernel/Makefile | 2 +-
arch/microblaze/kernel/timer.c | 326 ---------------
drivers/clocksource/Kconfig | 15 +
drivers/clocksource/Makefile | 1 +
drivers/clocksource/timer-xilinx.c | 650
I don't think this is the right way to go.
The first patch should be move current timer driver from microblaze to
generic location and then apply patches on the top based on what you
are
quoted
quoted
quoted
adding/fixing to be able to review every change separately.
When any issue happens it can be bisected and exact patch is
identified.
quoted
quoted
quoted
With this way we will end up in this patch and it will take a lot of
time to find where that problem is.
What parts would you like to see split? Fundamentally, this current
patch is a reimplementation of the driver. I think the only reasonable
split would be to add PWM support in a separate patch.
I do not think that genericizing the microblaze timer driver is an
integral part of adding PWM support. This is especially since you seem
opposed to using existing devicetree properties to inform the driver. I
am inclined to just add a patch adding a check for '#-pwm-cells' to the
existing driver and otherwise leave it untouched.
As I said I think the patches should be like this.
1. Cover existing DT binding based on current code.
2. Move time out of arch/microblaze to drivers/clocksource/ and even
enable it via Kconfig just for Microblaze.
3. Remove dependency on Microblaze and enable build for others. I have
seen at least one cpuinfo.cpu_clock_freq assignment. This code can be
likely completely removed or deprecate.
This could be deprecated, but cannot be removed since existing device
trees (e.g. qemu) have neither clocks nor clock-frequency properties.
Rob: Do we have any obligation to keep properties for other projects?
quoted
4. Make driver as module
5. Do whatever changes you want before adding pwm support
6. Extend DT binding doc for PWM support
7. Add PWM support
Frankly, I am inclined to just leave the microblaze timer as-is. The PWM
driver is completely independent. I have already put too much effort into
this driver, and I don't have the energy to continue working on the
microblaze timer.
I understand. I am actually using axi timer as pwm driver in one of my
project but never had time to upstream it because of couple of steps above.
We need to do it right based on steps listed above. If this is too much
work it will have to wait. I will NACK all attempts to add separate
driver for IP which we already support in the tree.
Thanks,
Michal
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Sean Anderson <hidden> Date: 2021-05-20 20:13:34
On 5/19/21 3:24 AM, Michal Simek wrote:
>
>
> On 5/18/21 12:15 AM, Sean Anderson wrote:
>>
>>
>> On 5/17/21 3:54 AM, Michal Simek wrote:
>>>
>>>
>>> On 5/14/21 4:40 PM, Sean Anderson wrote:
>>>>
>>>>
>>>> On 5/14/21 4:59 AM, Michal Simek wrote:
>>>>>
>>>>>
>>>>> On 5/11/21 9:12 PM, Sean Anderson wrote:
>>>>>> This adds generic clocksource and clockevent support for Xilinx
>>>> LogiCORE IP
>>>>>> AXI soft timers commonly found on Xilinx FPGAs. This timer is also the
>>>>>> primary timer for Microblaze processors. This commit also adds
>>>> support for
>>>>>> configuring this timer as a PWM (though this could be split off if
>>>>>> necessary). This whole driver lives in clocksource because it is
>>>> primarily
>>>>>> clocksource stuff now (even though it started out as a PWM driver). I
>>>> think
>>>>>> teasing apart the driver would not be worth it since they share so
>> many
>>>>>> functions.
>>>>>>
>>>>>> This driver configures timer 0 (which is always present) as a
>>>> clocksource,
>>>>>> and timer 1 (which might be missing) as a clockevent. I don't know if
>>>> this
>>>>>> is the correct priority for these timers, or whether we should be
>>>> using a
>>>>>> more dynamic allocation scheme.
>>>>>>
>>>>>> At the moment clock control is very basic: we just enable the clock
>>>> during
>>>>>> probe and pin the frequency. In the future, someone could add support
>>>> for
>>>>>> disabling the clock when not in use. Cascade mode is also unsupported.
>>>>>>
>>>>>> This driver was written with reference to Xilinx DS764 for v1.03.a
>> [1].
>>>>>>
>>>>>> [1]
>>>>
>> https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
>>
>>>>
>>>>>>
>>>>>> Signed-off-by: Sean Anderson [off-list ref]
>>>>>> ---
>>>>>> Please let me know if I should organize this differently or if it
>> should
>>>>>> be broken up.
>>>>>>
>>>>>> Changes in v3:
>>>>>> - Add clockevent and clocksource support
>>>>>> - Rewrite probe to only use a device_node, since timers may need to be
>>>>>> initialized before we have proper devices. This does bloat the
>>>> code a bit
>>>>>> since we can no longer rely on helpers such as dev_err_probe.
>> We also
>>>>>> cannot rely on device resources being free'd on failure, so we
>>>> must free
>>>>>> them manually.
>>>>>> - We now access registers through xilinx_timer_(read|write). This
>>>> allows us
>>>>>> to deal with endianness issues, as originally seen in the
>> microblaze
>>>>>> driver. CAVEAT EMPTOR: I have not tested this on big-endian!
>>>>>> - Remove old microblaze driver
>>>>>>
>>>>>> Changes in v2:
>>>>>> - Don't compile this module by default for arm64
>>>>>> - Add dependencies on COMMON_CLK and HAS_IOMEM
>>>>>> - Add comment explaining why we depend on !MICROBLAZE
>>>>>> - Add comment describing device
>>>>>> - Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
>>>>>> - Use NSEC_TO_SEC instead of defining our own
>>>>>> - Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by
>> Uwe
>>>>>> - Cast dividends to u64 to avoid overflow
>>>>>> - Check for over- and underflow when calculating TLR
>>>>>> - Set xilinx_pwm_ops.owner
>>>>>> - Don't set pwmchip.base to -1
>>>>>> - Check range of xlnx,count-width
>>>>>> - Ensure the clock is always running when the pwm is registered
>>>>>> - Remove debugfs file :l
>>>>>> - Report errors with dev_error_probe
>>>>>>
>>>>>> arch/microblaze/kernel/Makefile | 2 +-
>>>>>> arch/microblaze/kernel/timer.c | 326 ---------------
>>>>>> drivers/clocksource/Kconfig | 15 +
>>>>>> drivers/clocksource/Makefile | 1 +
>>>>>> drivers/clocksource/timer-xilinx.c | 650
>> +++++++++++++++++++++++++++++
>>>>>> 5 files changed, 667 insertions(+), 327 deletions(-)
>>>>>> delete mode 100644 arch/microblaze/kernel/timer.c
>>>>>> create mode 100644 drivers/clocksource/timer-xilinx.c
>>>>>
>>>>> I don't think this is the right way to go.
>>>>> The first patch should be move current timer driver from microblaze to
>>>>> generic location and then apply patches on the top based on what you
>> are
>>>>> adding/fixing to be able to review every change separately.
>>>>> When any issue happens it can be bisected and exact patch is
>> identified.
>>>>> With this way we will end up in this patch and it will take a lot of
>>>>> time to find where that problem is.
>>>>
>>>> What parts would you like to see split? Fundamentally, this current
>>>> patch is a reimplementation of the driver. I think the only reasonable
>>>> split would be to add PWM support in a separate patch.
>>>>
>>>> I do not think that genericizing the microblaze timer driver is an
>>>> integral part of adding PWM support. This is especially since you seem
>>>> opposed to using existing devicetree properties to inform the driver. I
>>>> am inclined to just add a patch adding a check for '#-pwm-cells' to the
>>>> existing driver and otherwise leave it untouched.
>>>
>>> As I said I think the patches should be like this.
>>> 1. Cover existing DT binding based on current code.
>>> 2. Move time out of arch/microblaze to drivers/clocksource/ and even
>>> enable it via Kconfig just for Microblaze.
>>> 3. Remove dependency on Microblaze and enable build for others. I have
>>> seen at least one cpuinfo.cpu_clock_freq assignment. This code can be
>>> likely completely removed or deprecate.
>>
>> This could be deprecated, but cannot be removed since existing device
>> trees (e.g. qemu) have neither clocks nor clock-frequency properties.
>
> Rob: Do we have any obligation to keep properties for other projects?
>
>
>>> 4. Make driver as module
>>> 5. Do whatever changes you want before adding pwm support
>>> 6. Extend DT binding doc for PWM support
>>> 7. Add PWM support
>>
>> Frankly, I am inclined to just leave the microblaze timer as-is. The PWM
>> driver is completely independent. I have already put too much effort into
>> this driver, and I don't have the energy to continue working on the
>> microblaze timer.
>
> I understand. I am actually using axi timer as pwm driver in one of my
> project but never had time to upstream it because of couple of steps above.
> We need to do it right based on steps listed above. If this is too much
> work it will have to wait. I will NACK all attempts to add separate
> driver for IP which we already support in the tree.
1. Many timers have separate clocksource and PWM drivers. E.g. samsung,
renesas TPU, etc. It is completely reasonable to keep separate
drivers for these purposes. There is no Linux requirement that each
device have only one driver, especially if it has multiple functions
or ways to be configured.
2. If you want to do work on a driver, I'm all for it. However, if you
have not yet submitted that work to the list, you should not gate
other work behind it. Saying that X feature must be gated behind Y
*even if X works completely independently of Y* is just stifling
development.
3. There is a clear desire for a PWM driver for this device. You, I, and
Alvaro have all written separate drivers for this device because we
want to use it as a PWM. By preventing merging this driver, you are
encouraging duplicate effort by the next person who wants to use this
device as a PWM, and sees that there is no driver in the tree.
--Sean
>
> Thanks,
> Michal
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Signed-off-by: Sean Anderson <redacted>
---
Please let me know if I should organize this differently or if it
should
quoted
quoted
quoted
quoted
be broken up.
Changes in v3:
- Add clockevent and clocksource support
- Rewrite probe to only use a device_node, since timers may need
to be
quoted
quoted
quoted
quoted
quoted
quoted
initialized before we have proper devices. This does bloat
the
quoted
quoted
quoted
quoted
code a bit
quoted
quoted
since we can no longer rely on helpers such as dev_err_probe.
We also
quoted
quoted
quoted
quoted
cannot rely on device resources being free'd on failure,
so we
quoted
quoted
quoted
quoted
must free
quoted
quoted
them manually.
- We now access registers through xilinx_timer_(read|write). This
allows us
quoted
quoted
to deal with endianness issues, as originally seen in the
microblaze
quoted
quoted
quoted
quoted
driver. CAVEAT EMPTOR: I have not tested this on big-endian!
- Remove old microblaze driver
Changes in v2:
- Don't compile this module by default for arm64
- Add dependencies on COMMON_CLK and HAS_IOMEM
- Add comment explaining why we depend on !MICROBLAZE
- Add comment describing device
- Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
- Use NSEC_TO_SEC instead of defining our own
- Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by
Uwe
quoted
quoted
quoted
quoted
- Cast dividends to u64 to avoid overflow
- Check for over- and underflow when calculating TLR
- Set xilinx_pwm_ops.owner
- Don't set pwmchip.base to -1
- Check range of xlnx,count-width
- Ensure the clock is always running when the pwm is registered
- Remove debugfs file :l
- Report errors with dev_error_probe
arch/microblaze/kernel/Makefile | 2 +-
arch/microblaze/kernel/timer.c | 326 ---------------
drivers/clocksource/Kconfig | 15 +
drivers/clocksource/Makefile | 1 +
drivers/clocksource/timer-xilinx.c | 650
I don't think this is the right way to go.
The first patch should be move current timer driver from
microblaze to
quoted
quoted
quoted
quoted
quoted
generic location and then apply patches on the top based on what you
are
quoted
quoted
quoted
adding/fixing to be able to review every change separately.
When any issue happens it can be bisected and exact patch is
identified.
quoted
quoted
quoted
With this way we will end up in this patch and it will take a lot of
time to find where that problem is.
What parts would you like to see split? Fundamentally, this current
patch is a reimplementation of the driver. I think the only reasonable
split would be to add PWM support in a separate patch.
I do not think that genericizing the microblaze timer driver is an
integral part of adding PWM support. This is especially since you seem
opposed to using existing devicetree properties to inform the
driver. I
quoted
quoted
quoted
quoted
am inclined to just add a patch adding a check for '#-pwm-cells' to
the
quoted
quoted
quoted
quoted
existing driver and otherwise leave it untouched.
As I said I think the patches should be like this.
1. Cover existing DT binding based on current code.
2. Move time out of arch/microblaze to drivers/clocksource/ and even
enable it via Kconfig just for Microblaze.
3. Remove dependency on Microblaze and enable build for others. I have
seen at least one cpuinfo.cpu_clock_freq assignment. This code can be
likely completely removed or deprecate.
This could be deprecated, but cannot be removed since existing device
trees (e.g. qemu) have neither clocks nor clock-frequency properties.
Rob: Do we have any obligation to keep properties for other projects?
quoted
quoted
4. Make driver as module
5. Do whatever changes you want before adding pwm support
6. Extend DT binding doc for PWM support
7. Add PWM support
Frankly, I am inclined to just leave the microblaze timer as-is. The PWM
driver is completely independent. I have already put too much effort
into
quoted
quoted
this driver, and I don't have the energy to continue working on the
microblaze timer.
I understand. I am actually using axi timer as pwm driver in one of my
project but never had time to upstream it because of couple of steps
above.
quoted
We need to do it right based on steps listed above. If this is too much
work it will have to wait. I will NACK all attempts to add separate
driver for IP which we already support in the tree.
1. Many timers have separate clocksource and PWM drivers. E.g. samsung,
renesas TPU, etc. It is completely reasonable to keep separate
drivers for these purposes. There is no Linux requirement that each
device have only one driver, especially if it has multiple functions
or ways to be configured.
It doesn't mean that it was done properly and correctly. Code
duplication is bad all the time.
2. If you want to do work on a driver, I'm all for it. However, if you
have not yet submitted that work to the list, you should not gate
other work behind it. Saying that X feature must be gated behind Y
*even if X works completely independently of Y* is just stifling
development.
I gave you guidance how I think this should be done. I am not gating you
from this work. Your patch is not working on Microblaze arch which is
what I maintain. And I don't want to go the route that we will have two
drivers for the same IP without integration. We were there in past and
it is just pain.
I am expecting that PWM guys will guide how this should be done
properly. I haven't heard any guidance on this yet.
Thierry/Uwe: Any comment?
3. There is a clear desire for a PWM driver for this device. You, I, and
Alvaro have all written separate drivers for this device because we
want to use it as a PWM. By preventing merging this driver, you are
encouraging duplicate effort by the next person who wants to use this
device as a PWM, and sees that there is no driver in the tree.
We should do it cleanly that it will be easy to maintain which is not by
creating two separate drivers or by switching to completely new driver.
Thanks,
Michal
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Sean Anderson <hidden> Date: 2021-05-24 18:34:27
On 5/24/21 3:00 AM, Michal Simek wrote:
>
>
> On 5/20/21 10:13 PM, Sean Anderson wrote:
>>
>>
>> On 5/19/21 3:24 AM, Michal Simek wrote:
>>>
>>>
>>> On 5/18/21 12:15 AM, Sean Anderson wrote:
>>>>
>>>>
>>>> On 5/17/21 3:54 AM, Michal Simek wrote:
>>>>>
>>>>>
>>>>> On 5/14/21 4:40 PM, Sean Anderson wrote:
>>>>>>
>>>>>>
>>>>>> On 5/14/21 4:59 AM, Michal Simek wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 5/11/21 9:12 PM, Sean Anderson wrote:
>>>>>>>> This adds generic clocksource and clockevent support for Xilinx
>>>>>> LogiCORE IP
>>>>>>>> AXI soft timers commonly found on Xilinx FPGAs. This timer is
>> also the
>>>>>>>> primary timer for Microblaze processors. This commit also adds
>>>>>> support for
>>>>>>>> configuring this timer as a PWM (though this could be split off if
>>>>>>>> necessary). This whole driver lives in clocksource because it is
>>>>>> primarily
>>>>>>>> clocksource stuff now (even though it started out as a PWM
>> driver). I
>>>>>> think
>>>>>>>> teasing apart the driver would not be worth it since they share so
>>>> many
>>>>>>>> functions.
>>>>>>>>
>>>>>>>> This driver configures timer 0 (which is always present) as a
>>>>>> clocksource,
>>>>>>>> and timer 1 (which might be missing) as a clockevent. I don't
>> know if
>>>>>> this
>>>>>>>> is the correct priority for these timers, or whether we should be
>>>>>> using a
>>>>>>>> more dynamic allocation scheme.
>>>>>>>>
>>>>>>>> At the moment clock control is very basic: we just enable the clock
>>>>>> during
>>>>>>>> probe and pin the frequency. In the future, someone could add
>> support
>>>>>> for
>>>>>>>> disabling the clock when not in use. Cascade mode is also
>> unsupported.
>>>>>>>>
>>>>>>>> This driver was written with reference to Xilinx DS764 for v1.03.a
>>>> [1].
>>>>>>>>
>>>>>>>> [1]
>>>>>>
>>>>
>> https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
>>
>>>>
>>>>>>
>>>>>>>>
>>>>>>>> Signed-off-by: Sean Anderson [off-list ref]
>>>>>>>> ---
>>>>>>>> Please let me know if I should organize this differently or if it
>>>> should
>>>>>>>> be broken up.
>>>>>>>>
>>>>>>>> Changes in v3:
>>>>>>>> - Add clockevent and clocksource support
>>>>>>>> - Rewrite probe to only use a device_node, since timers may need
>> to be
>>>>>>>> initialized before we have proper devices. This does bloat
>> the
>>>>>> code a bit
>>>>>>>> since we can no longer rely on helpers such as dev_err_probe.
>>>> We also
>>>>>>>> cannot rely on device resources being free'd on failure,
>> so we
>>>>>> must free
>>>>>>>> them manually.
>>>>>>>> - We now access registers through xilinx_timer_(read|write). This
>>>>>> allows us
>>>>>>>> to deal with endianness issues, as originally seen in the
>>>> microblaze
>>>>>>>> driver. CAVEAT EMPTOR: I have not tested this on big-endian!
>>>>>>>> - Remove old microblaze driver
>>>>>>>>
>>>>>>>> Changes in v2:
>>>>>>>> - Don't compile this module by default for arm64
>>>>>>>> - Add dependencies on COMMON_CLK and HAS_IOMEM
>>>>>>>> - Add comment explaining why we depend on !MICROBLAZE
>>>>>>>> - Add comment describing device
>>>>>>>> - Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
>>>>>>>> - Use NSEC_TO_SEC instead of defining our own
>>>>>>>> - Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by
>>>> Uwe
>>>>>>>> - Cast dividends to u64 to avoid overflow
>>>>>>>> - Check for over- and underflow when calculating TLR
>>>>>>>> - Set xilinx_pwm_ops.owner
>>>>>>>> - Don't set pwmchip.base to -1
>>>>>>>> - Check range of xlnx,count-width
>>>>>>>> - Ensure the clock is always running when the pwm is registered
>>>>>>>> - Remove debugfs file :l
>>>>>>>> - Report errors with dev_error_probe
>>>>>>>>
>>>>>>>> arch/microblaze/kernel/Makefile | 2 +-
>>>>>>>> arch/microblaze/kernel/timer.c | 326 ---------------
>>>>>>>> drivers/clocksource/Kconfig | 15 +
>>>>>>>> drivers/clocksource/Makefile | 1 +
>>>>>>>> drivers/clocksource/timer-xilinx.c | 650
>>>> +++++++++++++++++++++++++++++
>>>>>>>> 5 files changed, 667 insertions(+), 327 deletions(-)
>>>>>>>> delete mode 100644 arch/microblaze/kernel/timer.c
>>>>>>>> create mode 100644 drivers/clocksource/timer-xilinx.c
>>>>>>>
>>>>>>> I don't think this is the right way to go.
>>>>>>> The first patch should be move current timer driver from
>> microblaze to
>>>>>>> generic location and then apply patches on the top based on what you
>>>> are
>>>>>>> adding/fixing to be able to review every change separately.
>>>>>>> When any issue happens it can be bisected and exact patch is
>>>> identified.
>>>>>>> With this way we will end up in this patch and it will take a lot of
>>>>>>> time to find where that problem is.
>>>>>>
>>>>>> What parts would you like to see split? Fundamentally, this current
>>>>>> patch is a reimplementation of the driver. I think the only reasonable
>>>>>> split would be to add PWM support in a separate patch.
>>>>>>
>>>>>> I do not think that genericizing the microblaze timer driver is an
>>>>>> integral part of adding PWM support. This is especially since you seem
>>>>>> opposed to using existing devicetree properties to inform the
>> driver. I
>>>>>> am inclined to just add a patch adding a check for '#-pwm-cells' to
>> the
>>>>>> existing driver and otherwise leave it untouched.
>>>>>
>>>>> As I said I think the patches should be like this.
>>>>> 1. Cover existing DT binding based on current code.
>>>>> 2. Move time out of arch/microblaze to drivers/clocksource/ and even
>>>>> enable it via Kconfig just for Microblaze.
>>>>> 3. Remove dependency on Microblaze and enable build for others. I have
>>>>> seen at least one cpuinfo.cpu_clock_freq assignment. This code can be
>>>>> likely completely removed or deprecate.
>>>>
>>>> This could be deprecated, but cannot be removed since existing device
>>>> trees (e.g. qemu) have neither clocks nor clock-frequency properties.
>>>
>>> Rob: Do we have any obligation to keep properties for other projects?
>>>
>>>
>>>>> 4. Make driver as module
>>>>> 5. Do whatever changes you want before adding pwm support
>>>>> 6. Extend DT binding doc for PWM support
>>>>> 7. Add PWM support
>>>>
>>>> Frankly, I am inclined to just leave the microblaze timer as-is. The PWM
>>>> driver is completely independent. I have already put too much effort
>> into
>>>> this driver, and I don't have the energy to continue working on the
>>>> microblaze timer.
>>>
>>> I understand. I am actually using axi timer as pwm driver in one of my
>>> project but never had time to upstream it because of couple of steps
>> above.
>>> We need to do it right based on steps listed above. If this is too much
>>> work it will have to wait. I will NACK all attempts to add separate
>>> driver for IP which we already support in the tree.
>>
>> 1. Many timers have separate clocksource and PWM drivers. E.g. samsung,
>> renesas TPU, etc. It is completely reasonable to keep separate
>> drivers for these purposes. There is no Linux requirement that each
>> device have only one driver, especially if it has multiple functions
>> or ways to be configured.
>
> It doesn't mean that it was done properly and correctly. Code
> duplication is bad all the time.
IMO after doing all this there is not too much which can be reused. We
can reuse the read/write functions, the TLR calculations and the
processing of xlnx,counter-width and xlnx,one-timer. The timer probe is
likely much more cleanly implemented with timer_of_init. And not having
a platform device greatly complicates the PWM probe.
>
>> 2. If you want to do work on a driver, I'm all for it. However, if you
>> have not yet submitted that work to the list, you should not gate
>> other work behind it. Saying that X feature must be gated behind Y
>> *even if X works completely independently of Y* is just stifling
>> development.
>
> I gave you guidance how I think this should be done. I am not gating you
> from this work. Your patch is not working on Microblaze arch which is
> what I maintain.
I tested this on Microblaze qemu. What problems do you see?
--Sean
> And I don't want to go the route that we will have two
> drivers for the same IP without integration. We were there in past and
> it is just pain.
> I am expecting that PWM guys will guide how this should be done
> properly. I haven't heard any guidance on this yet.
> Thierry/Uwe: Any comment?
>
>
>> 3. There is a clear desire for a PWM driver for this device. You, I, and
>> Alvaro have all written separate drivers for this device because we
>> want to use it as a PWM. By preventing merging this driver, you are
>> encouraging duplicate effort by the next person who wants to use this
>> device as a PWM, and sees that there is no driver in the tree.
>
> We should do it cleanly that it will be easy to maintain which is not by
> creating two separate drivers or by switching to completely new driver.
>
> Thanks,
> Michal
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hello Sean, hello Michal,
On Mon, May 24, 2021 at 09:00:51AM +0200, Michal Simek wrote:
On 5/20/21 10:13 PM, Sean Anderson wrote:
quoted
On 5/19/21 3:24 AM, Michal Simek wrote:
quoted
On 5/18/21 12:15 AM, Sean Anderson wrote:
quoted
This could be deprecated, but cannot be removed since existing device
trees (e.g. qemu) have neither clocks nor clock-frequency properties.
Rob: Do we have any obligation to keep properties for other projects?
If a binding is in the wild and used to be documented, it has to stay.
quoted
quoted
quoted
quoted
4. Make driver as module
5. Do whatever changes you want before adding pwm support
6. Extend DT binding doc for PWM support
7. Add PWM support
Frankly, I am inclined to just leave the microblaze timer as-is. The PWM
driver is completely independent. I have already put too much effort into
this driver, and I don't have the energy to continue working on the
microblaze timer.
I understand. I am actually using axi timer as pwm driver in one of my
project but never had time to upstream it because of couple of steps above.
We need to do it right based on steps listed above. If this is too much
work it will have to wait. I will NACK all attempts to add separate
driver for IP which we already support in the tree.
1. Many timers have separate clocksource and PWM drivers. E.g. samsung,
renesas TPU, etc. It is completely reasonable to keep separate
drivers for these purposes. There is no Linux requirement that each
device have only one driver, especially if it has multiple functions
or ways to be configured.
It doesn't mean that it was done properly and correctly. Code
duplication is bad all the time.
IMHO it's not so much about code duplication. Yes, code duplication is
bad and should be prevented if possible. But it's more important to not
introduce surprises. So I think it should be obvious from reading the
device tree source which timer is used to provide the PWM. I don't care
much if this is from an extra property (like xilinx,provide-pwm),
overriding the compatible or some other explicit mechanism. IIUC in this
suggested patch the selection is implicit and so this isn't so nice.
quoted
2. If you want to do work on a driver, I'm all for it. However, if you
have not yet submitted that work to the list, you should not gate
other work behind it. Saying that X feature must be gated behind Y
*even if X works completely independently of Y* is just stifling
development.
I gave you guidance how I think this should be done. I am not gating you
from this work. Your patch is not working on Microblaze arch which is
what I maintain. And I don't want to go the route that we will have two
drivers for the same IP without integration. We were there in past and
it is just pain.
I am expecting that PWM guys will guide how this should be done
properly. I haven't heard any guidance on this yet.
Thierry/Uwe: Any comment?
Not sure I can and want to provide guidance here. This is not Perl, but
still TIMTOWTDI. If it was me who cared here, I'd look into the
auxiliary bus (Documentation/driver-api/auxiliary_bus.rst) to check if
it can help to solve this problem.
quoted
3. There is a clear desire for a PWM driver for this device. You, I, and
Alvaro have all written separate drivers for this device because we
want to use it as a PWM. By preventing merging this driver, you are
encouraging duplicate effort by the next person who wants to use this
device as a PWM, and sees that there is no driver in the tree.
We should do it cleanly that it will be easy to maintain which is not by
creating two separate drivers or by switching to completely new driver.
+1
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
From: Sean Anderson <hidden> Date: 2021-05-25 14:31:26
On 5/25/21 2:11 AM, Uwe Kleine-König wrote:
> Hello Sean, hello Michal,
>
> On Mon, May 24, 2021 at 09:00:51AM +0200, Michal Simek wrote:
>> On 5/20/21 10:13 PM, Sean Anderson wrote:
>>> On 5/19/21 3:24 AM, Michal Simek wrote:
>>>> On 5/18/21 12:15 AM, Sean Anderson wrote:
>>>>> This could be deprecated, but cannot be removed since existing device
>>>>> trees (e.g. qemu) have neither clocks nor clock-frequency properties.
>>>>
>>>> Rob: Do we have any obligation to keep properties for other projects?
>
> If a binding is in the wild and used to be documented, it has to stay.
>
>>>>>> 4. Make driver as module
>>>>>> 5. Do whatever changes you want before adding pwm support
>>>>>> 6. Extend DT binding doc for PWM support
>>>>>> 7. Add PWM support
>>>>>
>>>>> Frankly, I am inclined to just leave the microblaze timer as-is. The PWM
>>>>> driver is completely independent. I have already put too much effort into
>>>>> this driver, and I don't have the energy to continue working on the
>>>>> microblaze timer.
>>>>
>>>> I understand. I am actually using axi timer as pwm driver in one of my
>>>> project but never had time to upstream it because of couple of steps above.
>>>> We need to do it right based on steps listed above. If this is too much
>>>> work it will have to wait. I will NACK all attempts to add separate
>>>> driver for IP which we already support in the tree.
>>>
>>> 1. Many timers have separate clocksource and PWM drivers. E.g. samsung,
>>> renesas TPU, etc. It is completely reasonable to keep separate
>>> drivers for these purposes. There is no Linux requirement that each
>>> device have only one driver, especially if it has multiple functions
>>> or ways to be configured.
>>
>> It doesn't mean that it was done properly and correctly. Code
>> duplication is bad all the time.
>
> IMHO it's not so much about code duplication. Yes, code duplication is
> bad and should be prevented if possible. But it's more important to not
> introduce surprises. So I think it should be obvious from reading the
> device tree source which timer is used to provide the PWM. I don't care
> much if this is from an extra property (like xilinx,provide-pwm),
> overriding the compatible or some other explicit mechanism. IIUC in this
> suggested patch the selection is implicit and so this isn't so nice.
In this patch, the selection is by the presence of the xlnx,pwm
property. In the next revision, this will be changed to be the presence
of #pwm-cells (by the request of Rob).
>>> 2. If you want to do work on a driver, I'm all for it. However, if you
>>> have not yet submitted that work to the list, you should not gate
>>> other work behind it. Saying that X feature must be gated behind Y
>>> *even if X works completely independently of Y* is just stifling
>>> development.
>>
>> I gave you guidance how I think this should be done. I am not gating you
>> from this work. Your patch is not working on Microblaze arch which is
>> what I maintain. And I don't want to go the route that we will have two
>> drivers for the same IP without integration. We were there in past and
>> it is just pain.
>> I am expecting that PWM guys will guide how this should be done
>> properly. I haven't heard any guidance on this yet.
>> Thierry/Uwe: Any comment?
>
> Not sure I can and want to provide guidance here. This is not Perl, but
> still TIMTOWTDI. If it was me who cared here, I'd look into the
> auxiliary bus (Documentation/driver-api/auxiliary_bus.rst) to check if
> it can help to solve this problem.
I don't think this is the correct solution here.
> A key requirement for utilizing the auxiliary bus is that there is no
> dependency on a physical bus, device, register accesses or regmap
> support.
Since both PWM and timer drivers need register access, we cannot use the
auxiliary bus here. Further, timers must be initialized very early
during boot, before we even have devices, and cannot be unregistered.
Because of this, it only makes sense to bind one driver to the device.
>
>>> 3. There is a clear desire for a PWM driver for this device. You, I, and
>>> Alvaro have all written separate drivers for this device because we
>>> want to use it as a PWM. By preventing merging this driver, you are
>>> encouraging duplicate effort by the next person who wants to use this
>>> device as a PWM, and sees that there is no driver in the tree.
>>
>> We should do it cleanly that it will be easy to maintain which is not by
>> creating two separate drivers or by switching to completely new driver.
>
> +1
Ok, then you would like me to continue my current approach where both
drivers live in the same file?
--Sean
>
> Best regards
> Uwe
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Michal Simek <hidden> Date: 2021-06-16 12:12:40
Hi Uwe,
On 5/25/21 8:11 AM, Uwe Kleine-König wrote:
Hello Sean, hello Michal,
On Mon, May 24, 2021 at 09:00:51AM +0200, Michal Simek wrote:
quoted
On 5/20/21 10:13 PM, Sean Anderson wrote:
quoted
On 5/19/21 3:24 AM, Michal Simek wrote:
quoted
On 5/18/21 12:15 AM, Sean Anderson wrote:
quoted
This could be deprecated, but cannot be removed since existing device
trees (e.g. qemu) have neither clocks nor clock-frequency properties.
Rob: Do we have any obligation to keep properties for other projects?
If a binding is in the wild and used to be documented, it has to stay.
quoted
quoted
quoted
quoted
quoted
4. Make driver as module
5. Do whatever changes you want before adding pwm support
6. Extend DT binding doc for PWM support
7. Add PWM support
Frankly, I am inclined to just leave the microblaze timer as-is. The PWM
driver is completely independent. I have already put too much effort into
this driver, and I don't have the energy to continue working on the
microblaze timer.
I understand. I am actually using axi timer as pwm driver in one of my
project but never had time to upstream it because of couple of steps above.
We need to do it right based on steps listed above. If this is too much
work it will have to wait. I will NACK all attempts to add separate
driver for IP which we already support in the tree.
1. Many timers have separate clocksource and PWM drivers. E.g. samsung,
renesas TPU, etc. It is completely reasonable to keep separate
drivers for these purposes. There is no Linux requirement that each
device have only one driver, especially if it has multiple functions
or ways to be configured.
It doesn't mean that it was done properly and correctly. Code
duplication is bad all the time.
IMHO it's not so much about code duplication. Yes, code duplication is
bad and should be prevented if possible. But it's more important to not
introduce surprises. So I think it should be obvious from reading the
device tree source which timer is used to provide the PWM. I don't care
much if this is from an extra property (like xilinx,provide-pwm),
overriding the compatible or some other explicit mechanism. IIUC in this
suggested patch the selection is implicit and so this isn't so nice.
quoted
quoted
2. If you want to do work on a driver, I'm all for it. However, if you
have not yet submitted that work to the list, you should not gate
other work behind it. Saying that X feature must be gated behind Y
*even if X works completely independently of Y* is just stifling
development.
I gave you guidance how I think this should be done. I am not gating you
from this work. Your patch is not working on Microblaze arch which is
what I maintain. And I don't want to go the route that we will have two
drivers for the same IP without integration. We were there in past and
it is just pain.
I am expecting that PWM guys will guide how this should be done
properly. I haven't heard any guidance on this yet.
Thierry/Uwe: Any comment?
Not sure I can and want to provide guidance here. This is not Perl, but
still TIMTOWTDI. If it was me who cared here, I'd look into the
auxiliary bus (Documentation/driver-api/auxiliary_bus.rst) to check if
it can help to solve this problem.
I recently got patches for cadence TTC driver
(drivers/clocksource/timer-cadence-ttc.c) for PWM support too. It is the
second and very similar case. This driver is used on Zynq as clock
source and can be also use as PWM. I can't believe that there are no
other examples how to deal with these timers which are used for PWM
generation.
Thanks,
Michal
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Sean Anderson <hidden> Date: 2021-06-18 21:24:20
On 6/16/21 8:12 AM, Michal Simek wrote:
> Hi Uwe,
>
> On 5/25/21 8:11 AM, Uwe Kleine-König wrote:
>> Hello Sean, hello Michal,
>>
>> On Mon, May 24, 2021 at 09:00:51AM +0200, Michal Simek wrote:
>>> On 5/20/21 10:13 PM, Sean Anderson wrote:
>>>> On 5/19/21 3:24 AM, Michal Simek wrote:
>>>>> On 5/18/21 12:15 AM, Sean Anderson wrote:
>>>>>> This could be deprecated, but cannot be removed since existing device
>>>>>> trees (e.g. qemu) have neither clocks nor clock-frequency properties.
>>>>>
>>>>> Rob: Do we have any obligation to keep properties for other projects?
>>
>> If a binding is in the wild and used to be documented, it has to stay.
>>
>>>>>>> 4. Make driver as module
>>>>>>> 5. Do whatever changes you want before adding pwm support
>>>>>>> 6. Extend DT binding doc for PWM support
>>>>>>> 7. Add PWM support
>>>>>>
>>>>>> Frankly, I am inclined to just leave the microblaze timer as-is. The PWM
>>>>>> driver is completely independent. I have already put too much effort into
>>>>>> this driver, and I don't have the energy to continue working on the
>>>>>> microblaze timer.
>>>>>
>>>>> I understand. I am actually using axi timer as pwm driver in one of my
>>>>> project but never had time to upstream it because of couple of steps above.
>>>>> We need to do it right based on steps listed above. If this is too much
>>>>> work it will have to wait. I will NACK all attempts to add separate
>>>>> driver for IP which we already support in the tree.
>>>>
>>>> 1. Many timers have separate clocksource and PWM drivers. E.g. samsung,
>>>> renesas TPU, etc. It is completely reasonable to keep separate
>>>> drivers for these purposes. There is no Linux requirement that each
>>>> device have only one driver, especially if it has multiple functions
>>>> or ways to be configured.
>>>
>>> It doesn't mean that it was done properly and correctly. Code
>>> duplication is bad all the time.
>>
>> IMHO it's not so much about code duplication. Yes, code duplication is
>> bad and should be prevented if possible. But it's more important to not
>> introduce surprises. So I think it should be obvious from reading the
>> device tree source which timer is used to provide the PWM. I don't care
>> much if this is from an extra property (like xilinx,provide-pwm),
>> overriding the compatible or some other explicit mechanism. IIUC in this
>> suggested patch the selection is implicit and so this isn't so nice.
>>
>>>> 2. If you want to do work on a driver, I'm all for it. However, if you
>>>> have not yet submitted that work to the list, you should not gate
>>>> other work behind it. Saying that X feature must be gated behind Y
>>>> *even if X works completely independently of Y* is just stifling
>>>> development.
>>>
>>> I gave you guidance how I think this should be done. I am not gating you
>>> from this work. Your patch is not working on Microblaze arch which is
>>> what I maintain. And I don't want to go the route that we will have two
>>> drivers for the same IP without integration. We were there in past and
>>> it is just pain.
>>> I am expecting that PWM guys will guide how this should be done
>>> properly. I haven't heard any guidance on this yet.
>>> Thierry/Uwe: Any comment?
>>
>> Not sure I can and want to provide guidance here. This is not Perl, but
>> still TIMTOWTDI. If it was me who cared here, I'd look into the
>> auxiliary bus (Documentation/driver-api/auxiliary_bus.rst) to check if
>> it can help to solve this problem.
>
> I recently got patches for cadence TTC driver
> (drivers/clocksource/timer-cadence-ttc.c) for PWM support too. It is the
> second and very similar case. This driver is used on Zynq as clock
> source and can be also use as PWM. I can't believe that there are no
> other examples how to deal with these timers which are used for PWM
> generation.
>
The approach I took in v4 is that probe functions and driver callbacks
live in drivers/timer and drivers/pwm, and common functions live in
drivers/mfd (although I may move them to drivers/timer since Lee Jones
doesn't like them there).
I would greatly appreciate if you could review v4. It has been on the
list for three weeks now with no comments from either you or Uwe.
--Sean
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Michal Simek <hidden> Date: 2021-06-24 16:26:01
Hi,
On 6/18/21 11:24 PM, Sean Anderson wrote:
On 6/16/21 8:12 AM, Michal Simek wrote:
quoted
Hi Uwe,
On 5/25/21 8:11 AM, Uwe Kleine-König wrote:
quoted
Hello Sean, hello Michal,
On Mon, May 24, 2021 at 09:00:51AM +0200, Michal Simek wrote:
quoted
On 5/20/21 10:13 PM, Sean Anderson wrote:
quoted
On 5/19/21 3:24 AM, Michal Simek wrote:
quoted
On 5/18/21 12:15 AM, Sean Anderson wrote:
quoted
This could be deprecated, but cannot be removed since existing
device
quoted
quoted
quoted
quoted
quoted
quoted
trees (e.g. qemu) have neither clocks nor clock-frequency
properties.
quoted
quoted
quoted
quoted
quoted
Rob: Do we have any obligation to keep properties for other projects?
If a binding is in the wild and used to be documented, it has to stay.
quoted
quoted
quoted
quoted
quoted
4. Make driver as module
5. Do whatever changes you want before adding pwm support
6. Extend DT binding doc for PWM support
7. Add PWM support
Frankly, I am inclined to just leave the microblaze timer as-is.
The PWM
quoted
quoted
quoted
quoted
quoted
quoted
driver is completely independent. I have already put too much
effort into
quoted
quoted
quoted
quoted
quoted
quoted
this driver, and I don't have the energy to continue working on the
microblaze timer.
I understand. I am actually using axi timer as pwm driver in one
of my
quoted
quoted
quoted
quoted
quoted
project but never had time to upstream it because of couple of
steps above.
quoted
quoted
quoted
quoted
quoted
We need to do it right based on steps listed above. If this is too
much
quoted
quoted
quoted
quoted
quoted
work it will have to wait. I will NACK all attempts to add separate
driver for IP which we already support in the tree.
1. Many timers have separate clocksource and PWM drivers. E.g.
samsung,
quoted
quoted
quoted
quoted
renesas TPU, etc. It is completely reasonable to keep separate
drivers for these purposes. There is no Linux requirement that
each
quoted
quoted
quoted
quoted
device have only one driver, especially if it has multiple
functions
quoted
quoted
quoted
quoted
or ways to be configured.
It doesn't mean that it was done properly and correctly. Code
duplication is bad all the time.
IMHO it's not so much about code duplication. Yes, code duplication is
bad and should be prevented if possible. But it's more important to not
introduce surprises. So I think it should be obvious from reading the
device tree source which timer is used to provide the PWM. I don't care
much if this is from an extra property (like xilinx,provide-pwm),
overriding the compatible or some other explicit mechanism. IIUC in this
suggested patch the selection is implicit and so this isn't so nice.
quoted
quoted
2. If you want to do work on a driver, I'm all for it. However, if you
have not yet submitted that work to the list, you should not gate
other work behind it. Saying that X feature must be gated behind Y
*even if X works completely independently of Y* is just stifling
development.
I gave you guidance how I think this should be done. I am not gating
you
quoted
quoted
quoted
from this work. Your patch is not working on Microblaze arch which is
what I maintain. And I don't want to go the route that we will have two
drivers for the same IP without integration. We were there in past and
it is just pain.
I am expecting that PWM guys will guide how this should be done
properly. I haven't heard any guidance on this yet.
Thierry/Uwe: Any comment?
Not sure I can and want to provide guidance here. This is not Perl, but
still TIMTOWTDI. If it was me who cared here, I'd look into the
auxiliary bus (Documentation/driver-api/auxiliary_bus.rst) to check if
it can help to solve this problem.
I recently got patches for cadence TTC driver
(drivers/clocksource/timer-cadence-ttc.c) for PWM support too. It is the
second and very similar case. This driver is used on Zynq as clock
source and can be also use as PWM. I can't believe that there are no
other examples how to deal with these timers which are used for PWM
generation.
The approach I took in v4 is that probe functions and driver callbacks
live in drivers/timer and drivers/pwm, and common functions live in
drivers/mfd (although I may move them to drivers/timer since Lee Jones
doesn't like them there).
I would greatly appreciate if you could review v4. It has been on the
list for three weeks now with no comments from either you or Uwe.