From: Hans de Goede <hdegoede@redhat.com>
To: Brian Norris <computersforpeace@gmail.com>
Cc: Tejun Heo <tj@kernel.org>, Kishon Vijay Abraham I <kishon@ti.com>,
Rob Herring <robh+dt@kernel.org>, Pawel Moll <pawel.moll@arm.com>,
Mark Rutland <mark.rutland@arm.com>,
Ian Campbell <ijc+devicetree@hellion.org.uk>,
Kumar Gala <galak@codeaurora.org>,
Gregory Fong <gregory.0xf0@gmail.com>,
Florian Fainelli <f.fainelli@gmail.com>,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-ide@vger.kernel.org
Subject: Re: [PATCH 5/5] ARM: dts: brcmstb: add nodes for SATA controller and PHY
Date: Fri, 20 Mar 2015 09:48:42 +0100 [thread overview]
Message-ID: <550BDEEA.9080503@redhat.com> (raw)
In-Reply-To: <20150319191105.GU32500@ld-irv-0074>
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 <computersforpeace@gmail.com>
>>>>>> ---
>>>>>> 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
next prev parent reply other threads:[~2015-03-20 8:49 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-19 1:23 [PATCH 1/5] Documentation: devicetree: add Broadcom SATA binding Brian Norris
2015-03-19 1:23 ` [PATCH 2/5] Documentation: devicetree: add Broadcom SATA PHY binding Brian Norris
2015-03-19 1:23 ` [PATCH 3/5] ata: add Broadcom AHCI SATA3 driver for STB chips Brian Norris
2015-03-20 22:58 ` Florian Fainelli
2015-03-19 1:23 ` [PATCH 4/5] phy: add Broadcom SATA3 PHY driver for Broadcom STB SoCs Brian Norris
2015-03-20 23:02 ` Florian Fainelli
2015-03-21 9:09 ` Hans de Goede
2015-03-25 21:59 ` Kishon Vijay Abraham I
2015-03-28 0:28 ` Brian Norris
2015-03-31 6:01 ` Kishon Vijay Abraham I
2015-04-02 2:28 ` Brian Norris
2015-04-07 6:07 ` Kishon Vijay Abraham I
2015-04-07 18:35 ` Brian Norris
2015-03-19 1:23 ` [PATCH 5/5] ARM: dts: brcmstb: add nodes for SATA controller and PHY Brian Norris
2015-03-19 11:10 ` Hans de Goede
2015-03-19 15:53 ` Brian Norris
2015-03-19 17:02 ` Hans de Goede
2015-03-19 17:36 ` Brian Norris
2015-03-19 19:11 ` Brian Norris
2015-03-20 8:48 ` Hans de Goede [this message]
2015-03-19 11:33 ` Sergei Shtylyov
2015-03-19 15:58 ` Brian Norris
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=550BDEEA.9080503@redhat.com \
--to=hdegoede@redhat.com \
--cc=computersforpeace@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=f.fainelli@gmail.com \
--cc=galak@codeaurora.org \
--cc=gregory.0xf0@gmail.com \
--cc=ijc+devicetree@hellion.org.uk \
--cc=kishon@ti.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-ide@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=pawel.moll@arm.com \
--cc=robh+dt@kernel.org \
--cc=tj@kernel.org \
/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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).