From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Subject: Re: [PATCH 5/5] ARM: dts: brcmstb: add nodes for SATA controller and PHY Date: Fri, 20 Mar 2015 09:48:42 +0100 Message-ID: <550BDEEA.9080503@redhat.com> References: <1426728222-8197-1-git-send-email-computersforpeace@gmail.com> <1426728222-8197-5-git-send-email-computersforpeace@gmail.com> <550AAEA1.5080301@redhat.com> <20150319155358.GA19571@brian-ubuntu> <550B0118.3040104@redhat.com> <20150319173640.GS32500@ld-irv-0074> <20150319191105.GU32500@ld-irv-0074> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150319191105.GU32500@ld-irv-0074> Sender: linux-ide-owner@vger.kernel.org To: Brian Norris Cc: Tejun Heo , Kishon Vijay Abraham I , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Gregory Fong , Florian Fainelli , devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-ide@vger.kernel.org List-Id: devicetree@vger.kernel.org Hi, On 19-03-15 20:11, Brian Norris wrote: > Replying to myself, because I may or may not like having conversations > with myself :) > > On Thu, Mar 19, 2015 at 10:36:40AM -0700, Brian Norris wrote: >> On Thu, Mar 19, 2015 at 06:02:16PM +0100, Hans de Goede wrote: >>> On 19-03-15 16:53, Brian Norris wrote: >>>> On Thu, Mar 19, 2015 at 12:10:25PM +0100, Hans de Goede wrote: >>>>> On 19-03-15 02:23, Brian Norris wrote: >>>>>> Signed-off-by: Brian Norris >>>>>> --- >>>>>> Light dependency on: >>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/331921.html >>>>>> for the surrounding text. >>>>>> >>>>>> arch/arm/boot/dts/bcm7445.dtsi | 36 ++++++++++++++++++++++++++++++++++++ >>>>>> 1 file changed, 36 insertions(+) >>>>>> >>>>>> diff --git a/arch/arm/boot/dts/bcm7445.dtsi b/arch/arm/boot/dts/bcm7445.dtsi >>>>>> index 9eaeac8dce1b..7a7c4d8c2afe 100644 >>>>>> --- a/arch/arm/boot/dts/bcm7445.dtsi >>>>>> +++ b/arch/arm/boot/dts/bcm7445.dtsi >>>>>> @@ -108,6 +108,42 @@ >>>>>> brcm,int-map-mask = <0x25c>, <0x7000000>; >>>>>> brcm,int-fwd-mask = <0x70000>; >>>>>> }; >>>>>> + >>>>>> + sata@f045a000 { >>>>>> + compatible = "brcm,bcm7445-ahci", "brcm,sata3-ahci"; >>>>>> + reg-names = "ahci", "top-ctrl"; >>>>>> + reg = <0x45a000 0xa9c>, <0x458040 0x24>; >>>>> >>>>> Why not simply drop the second register range here, and the minimal top-ctrl >>>>> poking you need in the phy driver's phy_init function ? >>>> >>>> I agree it's a little ugly, but your recommended solution will not work. >>>> >>>> The 'top-ctrl' register range includes several SATA functionalities, >>>> some of which are required for the PHY and some of which are definitely >>>> required for the SATA driver. >>> >>> I see, but the phy driver is required for the SATA driver anyways, >>> and since the BUS_CTRL setting seems to be static it might just as >>> well be set by the phy driver. The phy driver also poking some >>> common sata glue bits like this busctrl register is not unheard of, >>> esp. when these glue bits are in the phy register range. > > I realized I *do* still have some reservations about moving the > SATA_TOP_CTRL register range under the PHY DT binding; it's because all > arguments for it seem to rest on descriptions of how the software would > *like* to handle it. It's not at all about describing the hardware > correctly. I had the same doubts myself when making the suggestion actually :) If the busctrl register purely influences the ahci functional block and not the phy functional block, then you are right. However if you look at the registermap, then doing as I suggest feels more natural as you get 2 distinct register blocks, one for ahci one for the phy, but if the one register in the phy range actually is a ahci register, then it would probably be more accurate to describe things that way ... > I still see SATA_TOP_CTRL as a register resource that belongs to the > SATA controller, not to the PHY. It just happens that it has a few > registers in it that are also for use in the PHY. > > So, to best describe the *hardware*, it seems we might split top-ctrl > into 3 portions, where the middle gets assigned to a phy description, > and the first and last belong to the SATA controller description. > > But to most easily describe how *software* would best handle them, we > might stick all the custom stuff (i.e., all of top-ctrl + phy) into the > PHY description. > > I still think that, practically speaking, the latter should work just > fine, and it's only a theoretical concern that suggests the former. > > Thoughts? I do not like your original proposal with the overlapping / conflicting resources. I'm fine with either alternative you suggest above. So unless someone else weighs in you get to chose which one you like best. Regards, Hans