From: Dan Murphy <dmurphy@ti.com> To: Tony Lindgren <tony@atomide.com> Cc: Arnd Bergmann <arnd@arndb.de>, linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, t-kristo@ti.com, s-anna@ti.com, p.zabel@pengutronix.de Subject: Re: [RFC 08/11] ARM: dts: am33xx: Add prcm_resets node Date: Wed, 30 Apr 2014 13:13:27 -0500 [thread overview] Message-ID: <53613D47.4000905@ti.com> (raw) In-Reply-To: <20140430181042.GC12362@atomide.com> Tony On 04/30/2014 01:10 PM, Tony Lindgren wrote: > * Dan Murphy <dmurphy@ti.com> [140430 11:00]: >> Tony and Arnd >> >> Thanks for the comments >> >> On 04/29/2014 07:22 PM, Tony Lindgren wrote: >>> * Arnd Bergmann <arnd@arndb.de> [140429 13:35]: >>>> On Tuesday 29 April 2014 15:19:47 Dan Murphy wrote: >>>>> + * AM33xx reset index for PRCM Module >>>>> + * >>>>> + * Copyright 2014 Texas Instruments Inc. >>>>> + * >>>>> + * This program is free software; you can redistribute it and/or modify >>>>> + * it under the terms of the GNU General Public License version 2 as >>>>> + * published by the Free Software Foundation. >>>>> + */ >>>>> + >>>>> +#ifndef _DT_BINDINGS_RESET_TI_AM33XX_H >>>>> +#define _DT_BINDINGS_RESET_TI_AM33XX_H >>>>> + >>>>> +#define RESET_DEVICE_RESET 0 >>>>> +#define RESET_GFX_RESET 1 >>>>> +#define RESET_PER_RESET 2 >>>>> + >>>>> +#endif >>>> Interfaces like this should only be used if you can't use hardware >>>> numbers, in general. If these numbers are in the data sheet, just >>>> put them directly into the dts file, as we do for interrupt numbers, >>>> gpio numbers, register offsets etc. >>>> >>>> If you have made them up to define an interface between the driver >>>> and DT because there is no usable hardare ID, I'd suggest just using >>>> a single file across all SoCs that have this driver, and have >>>> a unified name space. >>> Also, it's a bit unclear how the reset controller phandle is used >>> referenced and used by the consumer device.. Maybe setting that up >>> first in a Linux generic way is a good point starting point. >>> >>> Maybe something like this along the same way as clocks are set up >>> (completely untested): >>> >>> &reset1 { >>> iva_reset: reset1 { >>> reg = /bits/ 8 <0>; >>> }; >>> gfx_reset: reset1 { >>> reg = /bits/ 8 <1>; >>> }; >>> ... >>> }; >>> >>> &iva { >>> compatible = "ti,ivahd"; >>> resets = <&reset1 1>; >>> ... >>> }; >> I had something very similar to this when I was developing this driver but moved away from this. >> >> Following the clocks implementation I had a separate dtsi for resets for each device and had the data defined like so >> for each SoC. >> >> &prcm_resets { >> device_reset: device_reset { >> rstctrl_offs = <0x1104>; >> ctrl_bit-shift = <0>; >> rstst_offs = <0x1114>; >> sts_bit-shift = <0>; >> }; >> >> gpu_reset: gpu_reset { >> rstctrl_offs = <0x0D00>; >> ctrl_bit-shift = <3>; >> rstst_offs = <0x0D0C>; >> sts_bit-shift = <5>; >> }; >> }; >> >> And then any client interested in a specific reset driver would include this >> >> resets = <&prcm_resets &gpu_reset>; >> reset-names = "gpu_reset"; >> >> Our reset code would then retrieve the register data through the phandle instead of an index. >> >> Thoughts? > Using phandles makes sense here because it avoids the indexing. Indexing > has a problem of needing to be in sync with the .dts files and the > device driver(s). Using an index also easily causes misuse of virtual > indexes being added that no longer describe the hardware at all. Thanks. What about placing register data in the dts files? Is there any issue with this concept? Dan > Regards, > > Tony -- ------------------ Dan Murphy
WARNING: multiple messages have this Message-ID (diff)
From: dmurphy@ti.com (Dan Murphy) To: linux-arm-kernel@lists.infradead.org Subject: [RFC 08/11] ARM: dts: am33xx: Add prcm_resets node Date: Wed, 30 Apr 2014 13:13:27 -0500 [thread overview] Message-ID: <53613D47.4000905@ti.com> (raw) In-Reply-To: <20140430181042.GC12362@atomide.com> Tony On 04/30/2014 01:10 PM, Tony Lindgren wrote: > * Dan Murphy <dmurphy@ti.com> [140430 11:00]: >> Tony and Arnd >> >> Thanks for the comments >> >> On 04/29/2014 07:22 PM, Tony Lindgren wrote: >>> * Arnd Bergmann <arnd@arndb.de> [140429 13:35]: >>>> On Tuesday 29 April 2014 15:19:47 Dan Murphy wrote: >>>>> + * AM33xx reset index for PRCM Module >>>>> + * >>>>> + * Copyright 2014 Texas Instruments Inc. >>>>> + * >>>>> + * This program is free software; you can redistribute it and/or modify >>>>> + * it under the terms of the GNU General Public License version 2 as >>>>> + * published by the Free Software Foundation. >>>>> + */ >>>>> + >>>>> +#ifndef _DT_BINDINGS_RESET_TI_AM33XX_H >>>>> +#define _DT_BINDINGS_RESET_TI_AM33XX_H >>>>> + >>>>> +#define RESET_DEVICE_RESET 0 >>>>> +#define RESET_GFX_RESET 1 >>>>> +#define RESET_PER_RESET 2 >>>>> + >>>>> +#endif >>>> Interfaces like this should only be used if you can't use hardware >>>> numbers, in general. If these numbers are in the data sheet, just >>>> put them directly into the dts file, as we do for interrupt numbers, >>>> gpio numbers, register offsets etc. >>>> >>>> If you have made them up to define an interface between the driver >>>> and DT because there is no usable hardare ID, I'd suggest just using >>>> a single file across all SoCs that have this driver, and have >>>> a unified name space. >>> Also, it's a bit unclear how the reset controller phandle is used >>> referenced and used by the consumer device.. Maybe setting that up >>> first in a Linux generic way is a good point starting point. >>> >>> Maybe something like this along the same way as clocks are set up >>> (completely untested): >>> >>> &reset1 { >>> iva_reset: reset1 { >>> reg = /bits/ 8 <0>; >>> }; >>> gfx_reset: reset1 { >>> reg = /bits/ 8 <1>; >>> }; >>> ... >>> }; >>> >>> &iva { >>> compatible = "ti,ivahd"; >>> resets = <&reset1 1>; >>> ... >>> }; >> I had something very similar to this when I was developing this driver but moved away from this. >> >> Following the clocks implementation I had a separate dtsi for resets for each device and had the data defined like so >> for each SoC. >> >> &prcm_resets { >> device_reset: device_reset { >> rstctrl_offs = <0x1104>; >> ctrl_bit-shift = <0>; >> rstst_offs = <0x1114>; >> sts_bit-shift = <0>; >> }; >> >> gpu_reset: gpu_reset { >> rstctrl_offs = <0x0D00>; >> ctrl_bit-shift = <3>; >> rstst_offs = <0x0D0C>; >> sts_bit-shift = <5>; >> }; >> }; >> >> And then any client interested in a specific reset driver would include this >> >> resets = <&prcm_resets &gpu_reset>; >> reset-names = "gpu_reset"; >> >> Our reset code would then retrieve the register data through the phandle instead of an index. >> >> Thoughts? > Using phandles makes sense here because it avoids the indexing. Indexing > has a problem of needing to be in sync with the .dts files and the > device driver(s). Using an index also easily causes misuse of virtual > indexes being added that no longer describe the hardware at all. Thanks. What about placing register data in the dts files? Is there any issue with this concept? Dan > Regards, > > Tony -- ------------------ Dan Murphy
next prev parent reply other threads:[~2014-04-30 18:13 UTC|newest] Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-04-29 20:19 [RFC] TI Reset Driver adapted to the reset core Dan Murphy 2014-04-29 20:19 ` Dan Murphy 2014-04-29 20:19 ` [RFC 01/11] drivers: reset: TI: SoC reset controller support Dan Murphy 2014-04-29 20:19 ` Dan Murphy 2014-04-29 20:36 ` Arnd Bergmann 2014-04-29 20:36 ` Arnd Bergmann 2014-04-30 8:20 ` Philipp Zabel 2014-04-30 8:20 ` Philipp Zabel 2014-04-30 17:50 ` Dan Murphy 2014-04-30 17:50 ` Dan Murphy 2014-04-29 20:19 ` [RFC 02/11] drivers: reset: dra7: Add reset data for dra7xx Dan Murphy 2014-04-29 20:19 ` Dan Murphy 2014-04-29 20:19 ` [RFC 03/11] drivers: reset: omap5: Add reset data for omap5 Dan Murphy 2014-04-29 20:19 ` Dan Murphy 2014-04-29 20:19 ` [RFC 04/11] drivers: reset: am335: Add reset data for am335 Dan Murphy 2014-04-29 20:19 ` Dan Murphy 2014-04-29 20:19 ` [RFC 05/11] drivers: reset: am43xx: Add reset data for am43xx Dan Murphy 2014-04-29 20:19 ` Dan Murphy 2014-04-29 20:19 ` [RFC 06/11] ARM: OMAP: Add reset init to prcm file Dan Murphy 2014-04-29 20:19 ` Dan Murphy 2014-04-29 20:19 ` [RFC 07/11] ARM: TI: Describe the ti reset DT entries Dan Murphy 2014-04-29 20:19 ` Dan Murphy 2014-04-29 20:19 ` [RFC 08/11] ARM: dts: am33xx: Add prcm_resets node Dan Murphy 2014-04-29 20:19 ` Dan Murphy 2014-04-29 20:34 ` Arnd Bergmann 2014-04-29 20:34 ` Arnd Bergmann 2014-04-30 0:22 ` Tony Lindgren 2014-04-30 0:22 ` Tony Lindgren 2014-04-30 18:00 ` Dan Murphy 2014-04-30 18:00 ` Dan Murphy 2014-04-30 18:10 ` Tony Lindgren 2014-04-30 18:10 ` Tony Lindgren 2014-04-30 18:13 ` Dan Murphy [this message] 2014-04-30 18:13 ` Dan Murphy 2014-04-30 22:33 ` Tony Lindgren 2014-04-30 22:33 ` Tony Lindgren 2014-05-01 18:46 ` Dan Murphy 2014-05-01 18:46 ` Dan Murphy 2014-04-29 20:19 ` [RFC 09/11] ARM: dts: am4372: " Dan Murphy 2014-04-29 20:19 ` Dan Murphy 2014-04-29 20:19 ` [RFC 10/11] ARM: dts: dra7: Add prm_resets node Dan Murphy 2014-04-29 20:19 ` Dan Murphy 2014-04-29 20:19 ` [RFC 11/11] ARM: dts: omap5: " Dan Murphy 2014-04-29 20:19 ` Dan Murphy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=53613D47.4000905@ti.com \ --to=dmurphy@ti.com \ --cc=arnd@arndb.de \ --cc=devicetree@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-omap@vger.kernel.org \ --cc=p.zabel@pengutronix.de \ --cc=s-anna@ti.com \ --cc=t-kristo@ti.com \ --cc=tony@atomide.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.