| Message ID | 1524021226-18474-2-git-send-email-kever.yang@rock-chips.com |
|---|---|
| State | Accepted |
| Commit | f9ef5447863f88898db2e997a5e056bcdb8362a5 |
| Delegated to: | Philipp Tomsich |
| Headers | show |
| Series | [U-Boot,1/3] rockchip: rk3188: add -u-boot.dtsi for rock-rk3188 | expand |
> Add dts node for timer3. > Because of the rockchip timer can only KNOWN "dtd_rockchip_rk3368_timer" > with OF_PLATDATA enable, so we override its compatible to > "rockchip,rk3368-timer". > > Signed-off-by: Kever Yang <kever.yang@rock-chips.com> > --- > > arch/arm/dts/rk3188-radxarock-u-boot.dtsi | 6 ++++++ > arch/arm/dts/rk3188.dtsi | 6 ++++++ > 2 files changed, 12 insertions(+) > Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
Kever, > On 25 Apr 2018, at 12:04, Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote: > >> Add dts node for timer3. >> Because of the rockchip timer can only KNOWN "dtd_rockchip_rk3368_timer" >> with OF_PLATDATA enable, so we override its compatible to >> "rockchip,rk3368-timer". >> >> Signed-off-by: Kever Yang <kever.yang@rock-chips.com> >> --- >> >> arch/arm/dts/rk3188-radxarock-u-boot.dtsi | 6 ++++++ >> arch/arm/dts/rk3188.dtsi | 6 ++++++ >> 2 files changed, 12 insertions(+) >> > > Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> This looks like a work-around that we’ll have to revert eventually. I would instead extend the driver to recognise the ‘rockchip,rk3188-timer’ and ‘rockchip,rk3288-timer’ as well. Please confirm that both these .compatible strings should also be handled by the same driver and I’ll make the change when applying this series. Thanks, Philipp.
Hi Philipp, On 04/25/2018 06:17 PM, Dr. Philipp Tomsich wrote: > Kever, > >> On 25 Apr 2018, at 12:04, Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote: >> >>> Add dts node for timer3. >>> Because of the rockchip timer can only KNOWN "dtd_rockchip_rk3368_timer" >>> with OF_PLATDATA enable, so we override its compatible to >>> "rockchip,rk3368-timer". >>> >>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com> >>> --- >>> >>> arch/arm/dts/rk3188-radxarock-u-boot.dtsi | 6 ++++++ >>> arch/arm/dts/rk3188.dtsi | 6 ++++++ >>> 2 files changed, 12 insertions(+) >>> >> Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> > This looks like a work-around that we’ll have to revert eventually. > I would instead extend the driver to recognise the ‘rockchip,rk3188-timer’ > and ‘rockchip,rk3288-timer’ as well. > > Please confirm that both these .compatible strings should also be handled > by the same driver and I’ll make the change when applying this series. Do you mean you want patch like this? I do not like add lots #ifdef/#elif like this. +++ b/drivers/timer/rockchip_timer.c @@ -17,7 +17,11 @@ DECLARE_GLOBAL_DATA_PTR; #if CONFIG_IS_ENABLED(OF_PLATDATA) struct rockchip_timer_plat { +#ifdef CONFIG_ROCKCHIP_RK3368 struct dtd_rockchip_rk3368_timer dtd; +#elif CONFIG_ROCKCHIP_RK3188 + struct dtd_rockchip_rk3188_timer dtd; +#endif }; #endif @@ -153,6 +157,7 @@ static const struct timer_ops rockchip_timer_ops = { static const struct udevice_id rockchip_timer_ids[] = { { .compatible = "rockchip,rk3368-timer" }, + { .compatible = "rockchip,rk3188-timer" }, {} }; Thanks, - Kever > > Thanks, > Philipp.
> Add dts node for timer3. > Because of the rockchip timer can only KNOWN "dtd_rockchip_rk3368_timer" > with OF_PLATDATA enable, so we override its compatible to > "rockchip,rk3368-timer". > > Signed-off-by: Kever Yang <kever.yang@rock-chips.com> > Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> > --- > > arch/arm/dts/rk3188-radxarock-u-boot.dtsi | 6 ++++++ > arch/arm/dts/rk3188.dtsi | 6 ++++++ > 2 files changed, 12 insertions(+) > Applied to u-boot-rockchip, thanks!
> On 26 Apr 2018, at 04:50, Kever Yang <kever.yang@rock-chips.com> wrote: > > Hi Philipp, > > On 04/25/2018 06:17 PM, Dr. Philipp Tomsich wrote: >> Kever, >> >>> On 25 Apr 2018, at 12:04, Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote: >>> >>>> Add dts node for timer3. >>>> Because of the rockchip timer can only KNOWN "dtd_rockchip_rk3368_timer" >>>> with OF_PLATDATA enable, so we override its compatible to >>>> "rockchip,rk3368-timer". >>>> >>>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com> >>>> --- >>>> >>>> arch/arm/dts/rk3188-radxarock-u-boot.dtsi | 6 ++++++ >>>> arch/arm/dts/rk3188.dtsi | 6 ++++++ >>>> 2 files changed, 12 insertions(+) >>>> >>> Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> >> This looks like a work-around that we’ll have to revert eventually. >> I would instead extend the driver to recognise the ‘rockchip,rk3188-timer’ >> and ‘rockchip,rk3288-timer’ as well. >> >> Please confirm that both these .compatible strings should also be handled >> by the same driver and I’ll make the change when applying this series. > > Do you mean you want patch like this? I do not like add lots > #ifdef/#elif like this. > +++ b/drivers/timer/rockchip_timer.c > @@ -17,7 +17,11 @@ DECLARE_GLOBAL_DATA_PTR; > > #if CONFIG_IS_ENABLED(OF_PLATDATA) > struct rockchip_timer_plat { > +#ifdef CONFIG_ROCKCHIP_RK3368 > struct dtd_rockchip_rk3368_timer dtd; > +#elif CONFIG_ROCKCHIP_RK3188 > + struct dtd_rockchip_rk3188_timer dtd; > +#endif > }; > #endif The OF_PLATDATA implementation is really broken in this regard: the only fix I can think of would be to emit "struct dtd_…” for every compatible listed in the DTS. > @@ -153,6 +157,7 @@ static const struct timer_ops rockchip_timer_ops = { > > static const struct udevice_id rockchip_timer_ids[] = { > { .compatible = "rockchip,rk3368-timer" }, > + { .compatible = "rockchip,rk3188-timer" }, > {} > }; I actually referred only to this part (i.e. adding the .compatible).
> On 26 Apr 2018, at 09:14, Dr. Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote: > > >> On 26 Apr 2018, at 04:50, Kever Yang <kever.yang@rock-chips.com> wrote: >> >> Hi Philipp, >> >> On 04/25/2018 06:17 PM, Dr. Philipp Tomsich wrote: >>> Kever, >>> >>>> On 25 Apr 2018, at 12:04, Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote: >>>> >>>>> Add dts node for timer3. >>>>> Because of the rockchip timer can only KNOWN "dtd_rockchip_rk3368_timer" >>>>> with OF_PLATDATA enable, so we override its compatible to >>>>> "rockchip,rk3368-timer". >>>>> >>>>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com> >>>>> --- >>>>> >>>>> arch/arm/dts/rk3188-radxarock-u-boot.dtsi | 6 ++++++ >>>>> arch/arm/dts/rk3188.dtsi | 6 ++++++ >>>>> 2 files changed, 12 insertions(+) >>>>> >>>> Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> >>> This looks like a work-around that we’ll have to revert eventually. >>> I would instead extend the driver to recognise the ‘rockchip,rk3188-timer’ >>> and ‘rockchip,rk3288-timer’ as well. >>> >>> Please confirm that both these .compatible strings should also be handled >>> by the same driver and I’ll make the change when applying this series. >> >> Do you mean you want patch like this? I do not like add lots >> #ifdef/#elif like this. >> +++ b/drivers/timer/rockchip_timer.c >> @@ -17,7 +17,11 @@ DECLARE_GLOBAL_DATA_PTR; >> >> #if CONFIG_IS_ENABLED(OF_PLATDATA) >> struct rockchip_timer_plat { >> +#ifdef CONFIG_ROCKCHIP_RK3368 >> struct dtd_rockchip_rk3368_timer dtd; >> +#elif CONFIG_ROCKCHIP_RK3188 >> + struct dtd_rockchip_rk3188_timer dtd; >> +#endif >> }; >> #endif > > The OF_PLATDATA implementation is really broken in this regard: the > only fix I can think of would be to emit "struct dtd_…” for every compatible > listed in the DTS. I stand corrected: the OF_PLATDATA implementation (i.e. dtoc) already created #define entries for all aliases in the compatible-list of a DTS node. We’ll have to clean up our DTS files and this driver in the next release cycle and simply need to make sure that a common compatible is found across all devices (and the driver expects that common structure name). —Philipp.
diff --git a/arch/arm/dts/rk3188-radxarock-u-boot.dtsi b/arch/arm/dts/rk3188-radxarock-u-boot.dtsi index 5d650b1..26f5707 100644 --- a/arch/arm/dts/rk3188-radxarock-u-boot.dtsi +++ b/arch/arm/dts/rk3188-radxarock-u-boot.dtsi @@ -16,3 +16,9 @@ status = "okay"; u-boot,dm-spl; }; + +&timer3 { + compatible = "rockchip,rk3368-timer", "rockchip,rk3288-timer"; + u-boot,dm-spl; + clock-frequency = <24000000>; +}; diff --git a/arch/arm/dts/rk3188.dtsi b/arch/arm/dts/rk3188.dtsi index 1a06843..aeb5b80 100644 --- a/arch/arm/dts/rk3188.dtsi +++ b/arch/arm/dts/rk3188.dtsi @@ -123,6 +123,12 @@ }; }; + timer3: timer@2000e000 { + compatible = "rockchip,rk3188-timer", "rockchip,rk3288-timer"; + reg = <0x2000e000 0x20>; + interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>; + }; + usbphy: phy { compatible = "rockchip,rk3188-usb-phy", "rockchip,rk3288-usb-phy"; rockchip,grf = <&grf>;
Add dts node for timer3. Because of the rockchip timer can only KNOWN "dtd_rockchip_rk3368_timer" with OF_PLATDATA enable, so we override its compatible to "rockchip,rk3368-timer". Signed-off-by: Kever Yang <kever.yang@rock-chips.com> --- arch/arm/dts/rk3188-radxarock-u-boot.dtsi | 6 ++++++ arch/arm/dts/rk3188.dtsi | 6 ++++++ 2 files changed, 12 insertions(+)