| Message ID | 1426728222-8197-5-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
Hi, 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 ? This avoids the weird / ugly register overlap with the phy driver, and I think you can then just use the ahci_platform driver unmodified. > + interrupts = <GIC_SPI 30 0>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + sata0: sata-port@0 { > + reg = <0>; > + phys = <&sata_phy 0>; > + }; > + > + sata1: sata-port@1 { > + reg = <1>; > + phys = <&sata_phy 1>; > + }; > + }; > + > + sata_phy: sata-phy@f0458100 { > + compatible = "brcm,bcm7445-sata-phy", "brcm,phy-sata3"; > + reg = <0x458100 0x1e00>, <0x45804c 0x10>; Why not simply use: reg = <0x458000 0x2000>, to me it seems that what you should really be using here. > + reg-names = "phy", "port-ctrl"; > + #phy-cells = <1>; > + #address-cells = <0x1>; > + #size-cells = <0x0>; > + > + sata-phy@0 { > + reg = <0>; > + }; > + > + sata-phy@1 { > + reg = <1>; > + }; > + }; > }; > > smpboot { > Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 3/19/2015 4:23 AM, 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 { Hm, why the <unit-address> part of the name doesn't correspond to the "reg" prop? > + compatible = "brcm,bcm7445-ahci", "brcm,sata3-ahci"; > + reg-names = "ahci", "top-ctrl"; > + reg = <0x45a000 0xa9c>, <0x458040 0x24>; > + interrupts = <GIC_SPI 30 0>; > + #address-cells = <1>; > + #size-cells = <0>; [...] > + sata_phy: sata-phy@f0458100 { Same question here... > + compatible = "brcm,bcm7445-sata-phy", "brcm,phy-sata3"; > + reg = <0x458100 0x1e00>, <0x45804c 0x10>; [...] WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, Thanks for the review. 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. We have: 0x00 VERSION 0x04 BUS_CTRL 0x08 TP_CTRL 0x0C PHY_CTRL_1 0x10 PHY_CTRL_2 0x14 PHY_CTRL_3 0x18 PHY_CTRL_4 0x1C TP_OUT 0x20 CLIENT_INIT_CTRL We *definitely* need the BUS_CTRL register in the SATA driver, since it controls the endianness of the AHCI register set as well as a few other important bits we may use in the future. So we can't just "drop" the "minimal poking" and expect things to work, just because that makes the device tree look nicer :) The problem is what to do with the PHY_CTRL registers that are kept in the middle of the block. These really belong as part of the PHY power-on/power-off control sequence (i.e., PHY driver). Do you have any better suggestions that don't involve dropping the BUS_CTRL register from the SATA driver? > This avoids the weird / ugly register overlap with the phy driver, and I think you > can then just use the ahci_platform driver unmodified. > > >+ interrupts = <GIC_SPI 30 0>; > >+ #address-cells = <1>; > >+ #size-cells = <0>; > >+ > >+ sata0: sata-port@0 { > >+ reg = <0>; > >+ phys = <&sata_phy 0>; > >+ }; > >+ > >+ sata1: sata-port@1 { > >+ reg = <1>; > >+ phys = <&sata_phy 1>; > >+ }; > >+ }; > >+ > >+ sata_phy: sata-phy@f0458100 { > >+ compatible = "brcm,bcm7445-sata-phy", "brcm,phy-sata3"; > >+ reg = <0x458100 0x1e00>, <0x45804c 0x10>; > > Why not simply use: reg = <0x458000 0x2000>, to me it seems that what you should > really be using here. 0x458000 to 0x45800f and 0x458080 to 0x4580af belong to a debug interrupt controller, which handles bus error handling. It's currently unused, and it definitely doesn't belong here. The 'phy' register range is actually documented as two sets of identical registers: 0x458100 - 0x458fff Port0 SATA PHY registers 0x459100 - 0x459fff Port1 SATA PHY registers with a hole between them. I definitely don't want to do the combining that you suggested, but I could probably split them apart if that really helps... (BTW, I realized I have my math wrong, and each port has length 0xf00, not 0xe00. So the range I posted should actually be <0x458100 0x1f00>.) > >+ reg-names = "phy", "port-ctrl"; > >+ #phy-cells = <1>; > >+ #address-cells = <0x1>; > >+ #size-cells = <0x0>; > >+ > >+ sata-phy@0 { > >+ reg = <0>; > >+ }; > >+ > >+ sata-phy@1 { > >+ reg = <1>; > >+ }; > >+ }; > > }; > > > > smpboot { > > Brian -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sergei, On Thu, Mar 19, 2015 at 02:33:32PM +0300, Sergei Shtylyov wrote: > Hello. > > On 3/19/2015 4:23 AM, 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 { > > Hm, why the <unit-address> part of the name doesn't correspond to the > "reg" prop? Whoops. This .dtsi file is confusing, since none of the Broadcom DTS's I deal with have the 'ranges' property building in an 0xf0000000 offset in the /rdb node. All our DTS's give absolute register addresses, not relative. This discrepancy is a copy-and-paste where I fixed the functional aspect (the 'reg' property) but overlooked the cosmetic (the name / unit-address). We considered just reworking the /rdb node so that it drops the 0xf0000000 offset, in order to be more consistent and to avoid "mistakes" like this. I could either do that (and use 0xf045a000 in both places) or just fix the unit address you pointed out. > >+ compatible = "brcm,bcm7445-ahci", "brcm,sata3-ahci"; > >+ reg-names = "ahci", "top-ctrl"; > >+ reg = <0x45a000 0xa9c>, <0x458040 0x24>; > >+ interrupts = <GIC_SPI 30 0>; > >+ #address-cells = <1>; > >+ #size-cells = <0>; > [...] > >+ sata_phy: sata-phy@f0458100 { > > Same question here... Same answer :) > >+ compatible = "brcm,bcm7445-sata-phy", "brcm,phy-sata3"; > >+ reg = <0x458100 0x1e00>, <0x45804c 0x10>; > [...] Thanks for the review. Brian -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 19-03-15 16:53, Brian Norris wrote: > Hi Hans, > > Thanks for the review. > > 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. > We have: > > 0x00 VERSION > 0x04 BUS_CTRL > 0x08 TP_CTRL > 0x0C PHY_CTRL_1 > 0x10 PHY_CTRL_2 > 0x14 PHY_CTRL_3 > 0x18 PHY_CTRL_4 > 0x1C TP_OUT > 0x20 CLIENT_INIT_CTRL > > We *definitely* need the BUS_CTRL register in the SATA driver, since it > controls the endianness of the AHCI register set as well as a few other > important bits we may use in the future. Are these bits non static, e.g. something which you may want to change at another time then init/shutdown/suspend/resume ? If they are static I still think this all clearly belongs in the phy driver, since this looks a lot like it is a single hardware block. If otoh these other bits may need runtime poking for e.g. sata error recovery, then I can understand why you want the bus ctrl register in the sata driver, but in that case it should only be mapped by the sata driver, and the phy driver register ranges should not cover it. > So we can't just "drop" the > "minimal poking" and expect things to work, just because that makes the > device tree look nicer :) I was not suggesting to drop it I was suggesting moving the poke to phy_init, and given the registermap you've shown above I still think that this belongs more in the phy driver then anywhere else. > The problem is what to do with the PHY_CTRL registers that are kept in > the middle of the block. These really belong as part of the PHY > power-on/power-off control sequence (i.e., PHY driver). > > Do you have any better suggestions that don't involve dropping the > BUS_CTRL register from the SATA driver? Nope, if you really believe that the bus-ctrl register should be part of the sata driver, then please just split the register ranges at a per register level, to remove the overlap + the hack of not claiming the resource on one side. >> This avoids the weird / ugly register overlap with the phy driver, and I think you >> can then just use the ahci_platform driver unmodified. >> >>> + interrupts = <GIC_SPI 30 0>; >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + >>> + sata0: sata-port@0 { >>> + reg = <0>; >>> + phys = <&sata_phy 0>; >>> + }; >>> + >>> + sata1: sata-port@1 { >>> + reg = <1>; >>> + phys = <&sata_phy 1>; >>> + }; >>> + }; >>> + >>> + sata_phy: sata-phy@f0458100 { >>> + compatible = "brcm,bcm7445-sata-phy", "brcm,phy-sata3"; >>> + reg = <0x458100 0x1e00>, <0x45804c 0x10>; >> >> Why not simply use: reg = <0x458000 0x2000>, to me it seems that what you should >> really be using here. > > 0x458000 to 0x45800f and 0x458080 to 0x4580af belong to a debug > interrupt controller, which handles bus error handling. It's currently > unused, and it definitely doesn't belong here. > > The 'phy' register range is actually documented as two sets of identical > registers: > > 0x458100 - 0x458fff Port0 SATA PHY registers > 0x459100 - 0x459fff Port1 SATA PHY registers > > with a hole between them. I definitely don't want to do the combining > that you suggested, but I could probably split them apart if that really > helps... If this is the documented register range, then I think that splitting them makes sense, and assuming that the init code for both ports is the same it may even make for cleaner code. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > > >We have: > > > >0x00 VERSION > >0x04 BUS_CTRL > >0x08 TP_CTRL > >0x0C PHY_CTRL_1 > >0x10 PHY_CTRL_2 > >0x14 PHY_CTRL_3 > >0x18 PHY_CTRL_4 > >0x1C TP_OUT > >0x20 CLIENT_INIT_CTRL > > > >We *definitely* need the BUS_CTRL register in the SATA driver, since it > >controls the endianness of the AHCI register set as well as a few other > >important bits we may use in the future. > > Are these bits non static, e.g. something which you may want to change at > another time then init/shutdown/suspend/resume ? If they are static I still > think this all clearly belongs in the phy driver, since this looks a lot > like it is a single hardware block. They are mostly static. I can't see right now anything (outside of the PHY ctrl registers) that might ever need handled dynamically. > If otoh these other bits may need runtime poking for e.g. sata error recovery, > then I can understand why you want the bus ctrl register in the sata driver, > but in that case it should only be mapped by the sata driver, and the phy driver > register ranges should not cover it. > > >So we can't just "drop" the > >"minimal poking" and expect things to work, just because that makes the > >device tree look nicer :) > > I was not suggesting to drop it I was suggesting moving the poke to phy_init, > and given the registermap you've shown above I still think that this belongs > more in the phy driver then anywhere else. Implicit in my statements so far was the assumption that the AHCI platform driver would actually use info from the AHCI registers before bringing up the PHY. For instance, my driver used to do something like this: 0. configure AHCI register endianness 1. check AHCI HOST_PORTS_IMPL 2. enable phy(s) for port(s) that are available (This would automatically account for cases where one or more port(s) are OTP'd out.) Such a sequence would require that the AHCI registers are prepared for use before the PHY driver ever comes into play. But I now see that such a sequence is not done, and that your suggestion would actually work. The OTP cases can be handled by disabling the PHY subnodes in the device tree. BTW, BUS_CTRL also contains a bit for allowing re-configuration of the AHCI CAPS register, and we'll probably need to use this soon. I see that this would probably actually be a PHY-dependent operation, since we will have some SoCs where features are buggy and should be disabled, and conversely other SoCs where new features are disabled by default, but can be enabled after silicon verification confirms they are "good." (Particularly: AHCI link power management.) I'm not sure whether these would be properties of the SATA driver or of the PHY driver. All in all, I think I'm leaning toward your suggestion of moving this all to the phy driver, and just using 'generic-ahci' directly. I would, of course, still like to define a 'brcm,bcm7445-ahci' compatible string, in case we need to differentiate from other 'generic' AHCI eventually. > >The problem is what to do with the PHY_CTRL registers that are kept in > >the middle of the block. These really belong as part of the PHY > >power-on/power-off control sequence (i.e., PHY driver). > > > >Do you have any better suggestions that don't involve dropping the > >BUS_CTRL register from the SATA driver? > > Nope, if you really believe that the bus-ctrl register should be part of > the sata driver, then please just split the register ranges at a per register > level, to remove the overlap + the hack of not claiming the resource on one side. I though of that earlier, but that's even uglier IMO :) Unless my comments earlier in the email change, I think I'll lean toward your former suggestion, actually. > >>This avoids the weird / ugly register overlap with the phy driver, and I think you > >>can then just use the ahci_platform driver unmodified. > >> > >>>+ interrupts = <GIC_SPI 30 0>; > >>>+ #address-cells = <1>; > >>>+ #size-cells = <0>; > >>>+ > >>>+ sata0: sata-port@0 { > >>>+ reg = <0>; > >>>+ phys = <&sata_phy 0>; > >>>+ }; > >>>+ > >>>+ sata1: sata-port@1 { > >>>+ reg = <1>; > >>>+ phys = <&sata_phy 1>; > >>>+ }; > >>>+ }; > >>>+ > >>>+ sata_phy: sata-phy@f0458100 { > >>>+ compatible = "brcm,bcm7445-sata-phy", "brcm,phy-sata3"; > >>>+ reg = <0x458100 0x1e00>, <0x45804c 0x10>; > >> > >>Why not simply use: reg = <0x458000 0x2000>, to me it seems that what you should > >>really be using here. > > > >0x458000 to 0x45800f and 0x458080 to 0x4580af belong to a debug > >interrupt controller, which handles bus error handling. It's currently > >unused, and it definitely doesn't belong here. > > > >The 'phy' register range is actually documented as two sets of identical > >registers: > > > >0x458100 - 0x458fff Port0 SATA PHY registers > >0x459100 - 0x459fff Port1 SATA PHY registers > > > >with a hole between them. I definitely don't want to do the combining > >that you suggested, but I could probably split them apart if that really > >helps... > > If this is the documented register range, then I think that splitting them makes > sense, and assuming that the init code for both ports is the same it may > even make for cleaner code. Eh, it doesn't do too much for the code. We'll just now have to grab two different resources "by name", and then do the register reads/writes by pulling from two different __iomem regions, instead of a single one plus an offset of (0x1000 * portnum). Not much difference. But yes, I can split the regions if that helps make the description better (TM). Brian -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 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? > > >We have: > > > > > >0x00 VERSION > > >0x04 BUS_CTRL > > >0x08 TP_CTRL > > >0x0C PHY_CTRL_1 > > >0x10 PHY_CTRL_2 > > >0x14 PHY_CTRL_3 > > >0x18 PHY_CTRL_4 > > >0x1C TP_OUT > > >0x20 CLIENT_INIT_CTRL [snip rest] Brian -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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>; + interrupts = <GIC_SPI 30 0>; + #address-cells = <1>; + #size-cells = <0>; + + sata0: sata-port@0 { + reg = <0>; + phys = <&sata_phy 0>; + }; + + sata1: sata-port@1 { + reg = <1>; + phys = <&sata_phy 1>; + }; + }; + + sata_phy: sata-phy@f0458100 { + compatible = "brcm,bcm7445-sata-phy", "brcm,phy-sata3"; + reg = <0x458100 0x1e00>, <0x45804c 0x10>; + reg-names = "phy", "port-ctrl"; + #phy-cells = <1>; + #address-cells = <0x1>; + #size-cells = <0x0>; + + sata-phy@0 { + reg = <0>; + }; + + sata-phy@1 { + reg = <1>; + }; + }; }; smpboot {
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(+)