| Message ID | 20170627132145.28159-5-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Platforms/AMD/Styx: various Cello related fixes | expand |
On Tue, Jun 27, 2017 at 01:21:39PM +0000, Ard Biesheuvel wrote: > The logic regarding how the various SMMUs are exposed in the device tree > is inverted, in the sense that they are present in the static DTB image, > and are removed if no SMMU support is requested. > > However, the logic is flawed in the sense that it did not remove SMMUs > for hardware that is not there to begin with, i.e., the XGBE network > ports on Cello/Softiron 1000 or the second SATA controller on B1 silicon. > > So fix that. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c b/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c > index 093db6517c1a..0da00655396e 100644 > --- a/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c > +++ b/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c > @@ -261,11 +261,18 @@ SetSocIdStatus ( > if (!PcdGetBool (PcdEnableSmmus)) { > DisableSmmu (Fdt, "iommu-map", "/smb/smmu@e0a00000", "/smb/pcie@f0000000"); > DisableSmmu (Fdt, "iommus", "/smb/smmu@e0200000", "/smb/sata@e0300000"); > + } > + > + if (!PcdGetBool (PcdEnableSmmus) || !IsRevB1 || FixedPcdGet8 (PcdSata1PortCount) == 0) { > DisableSmmu (Fdt, "iommus", "/smb/smmu@e0c00000", "/smb/sata@e0d00000"); > + } > + > #if DO_XGBE > + if (!PcdGetBool (PcdEnableSmmus)) > +#endif > + { > DisableSmmu (Fdt, "iommus", "/smb/smmu@e0600000", "/smb/xgmac@e0700000"); > DisableSmmu (Fdt, "iommus", "/smb/smmu@e0800000", "/smb/xgmac@e0900000"); > -#endif > } That's a little bit filthy. Could you do #if !DO_XBGE Around the whole new if-statement instead? / Leif > } > > -- > 2.9.3 >
On 27 June 2017 at 16:08, Leif Lindholm <leif.lindholm@linaro.org> wrote: > On Tue, Jun 27, 2017 at 01:21:39PM +0000, Ard Biesheuvel wrote: >> The logic regarding how the various SMMUs are exposed in the device tree >> is inverted, in the sense that they are present in the static DTB image, >> and are removed if no SMMU support is requested. >> >> However, the logic is flawed in the sense that it did not remove SMMUs >> for hardware that is not there to begin with, i.e., the XGBE network >> ports on Cello/Softiron 1000 or the second SATA controller on B1 silicon. >> >> So fix that. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c b/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c >> index 093db6517c1a..0da00655396e 100644 >> --- a/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c >> +++ b/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c >> @@ -261,11 +261,18 @@ SetSocIdStatus ( >> if (!PcdGetBool (PcdEnableSmmus)) { >> DisableSmmu (Fdt, "iommu-map", "/smb/smmu@e0a00000", "/smb/pcie@f0000000"); >> DisableSmmu (Fdt, "iommus", "/smb/smmu@e0200000", "/smb/sata@e0300000"); >> + } >> + >> + if (!PcdGetBool (PcdEnableSmmus) || !IsRevB1 || FixedPcdGet8 (PcdSata1PortCount) == 0) { >> DisableSmmu (Fdt, "iommus", "/smb/smmu@e0c00000", "/smb/sata@e0d00000"); >> + } >> + >> #if DO_XGBE >> + if (!PcdGetBool (PcdEnableSmmus)) >> +#endif >> + { >> DisableSmmu (Fdt, "iommus", "/smb/smmu@e0600000", "/smb/xgmac@e0700000"); >> DisableSmmu (Fdt, "iommus", "/smb/smmu@e0800000", "/smb/xgmac@e0900000"); >> -#endif >> } > > That's a little bit filthy. > Could you do > > #if !DO_XBGE > > Around the whole new if-statement instead? > Not really. What I actually need is if (!PcdGetBool (PcdEnableSmmus) || !DO_XGBE) which is not equivalent. However, this breaks due to the fact the DO_XGBE is not defined to 0 but simply not defined at all.
On 27 June 2017 at 16:11, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 27 June 2017 at 16:08, Leif Lindholm <leif.lindholm@linaro.org> wrote: >> On Tue, Jun 27, 2017 at 01:21:39PM +0000, Ard Biesheuvel wrote: >>> The logic regarding how the various SMMUs are exposed in the device tree >>> is inverted, in the sense that they are present in the static DTB image, >>> and are removed if no SMMU support is requested. >>> >>> However, the logic is flawed in the sense that it did not remove SMMUs >>> for hardware that is not there to begin with, i.e., the XGBE network >>> ports on Cello/Softiron 1000 or the second SATA controller on B1 silicon. >>> >>> So fix that. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> --- >>> Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c | 9 ++++++++- >>> 1 file changed, 8 insertions(+), 1 deletion(-) >>> >>> diff --git a/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c b/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c >>> index 093db6517c1a..0da00655396e 100644 >>> --- a/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c >>> +++ b/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c >>> @@ -261,11 +261,18 @@ SetSocIdStatus ( >>> if (!PcdGetBool (PcdEnableSmmus)) { >>> DisableSmmu (Fdt, "iommu-map", "/smb/smmu@e0a00000", "/smb/pcie@f0000000"); >>> DisableSmmu (Fdt, "iommus", "/smb/smmu@e0200000", "/smb/sata@e0300000"); >>> + } >>> + >>> + if (!PcdGetBool (PcdEnableSmmus) || !IsRevB1 || FixedPcdGet8 (PcdSata1PortCount) == 0) { >>> DisableSmmu (Fdt, "iommus", "/smb/smmu@e0c00000", "/smb/sata@e0d00000"); >>> + } >>> + >>> #if DO_XGBE >>> + if (!PcdGetBool (PcdEnableSmmus)) >>> +#endif >>> + { >>> DisableSmmu (Fdt, "iommus", "/smb/smmu@e0600000", "/smb/xgmac@e0700000"); >>> DisableSmmu (Fdt, "iommus", "/smb/smmu@e0800000", "/smb/xgmac@e0900000"); >>> -#endif >>> } >> >> That's a little bit filthy. >> Could you do >> >> #if !DO_XBGE >> >> Around the whole new if-statement instead? >> > > Not really. What I actually need is > > if (!PcdGetBool (PcdEnableSmmus) || !DO_XGBE) > > which is not equivalent. However, this breaks due to the fact the > DO_XGBE is not defined to 0 but simply not defined at all. How about #if DO_XGBE DisableXgbeSmmus = !PcdGetBool (PcdEnableSmmus); #else DisableXgbeSmmus = TRUE; #endif if (DisableXgbeSmmus) { DisableSmmu (Fdt, "iommus", "/smb/smmu@e0600000", "/smb/xgmac@e0700000"); DisableSmmu (Fdt, "iommus", "/smb/smmu@e0800000", "/smb/xgmac@e0900000"); } ?
On Tue, Jun 27, 2017 at 05:00:12PM +0000, Ard Biesheuvel wrote: > On 27 June 2017 at 16:11, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > On 27 June 2017 at 16:08, Leif Lindholm <leif.lindholm@linaro.org> wrote: > >> On Tue, Jun 27, 2017 at 01:21:39PM +0000, Ard Biesheuvel wrote: > >>> The logic regarding how the various SMMUs are exposed in the device tree > >>> is inverted, in the sense that they are present in the static DTB image, > >>> and are removed if no SMMU support is requested. > >>> > >>> However, the logic is flawed in the sense that it did not remove SMMUs > >>> for hardware that is not there to begin with, i.e., the XGBE network > >>> ports on Cello/Softiron 1000 or the second SATA controller on B1 silicon. > >>> > >>> So fix that. > >>> > >>> Contributed-under: TianoCore Contribution Agreement 1.0 > >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >>> --- > >>> Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c | 9 ++++++++- > >>> 1 file changed, 8 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c b/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c > >>> index 093db6517c1a..0da00655396e 100644 > >>> --- a/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c > >>> +++ b/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c > >>> @@ -261,11 +261,18 @@ SetSocIdStatus ( > >>> if (!PcdGetBool (PcdEnableSmmus)) { > >>> DisableSmmu (Fdt, "iommu-map", "/smb/smmu@e0a00000", "/smb/pcie@f0000000"); > >>> DisableSmmu (Fdt, "iommus", "/smb/smmu@e0200000", "/smb/sata@e0300000"); > >>> + } > >>> + > >>> + if (!PcdGetBool (PcdEnableSmmus) || !IsRevB1 || FixedPcdGet8 (PcdSata1PortCount) == 0) { > >>> DisableSmmu (Fdt, "iommus", "/smb/smmu@e0c00000", "/smb/sata@e0d00000"); > >>> + } > >>> + > >>> #if DO_XGBE > >>> + if (!PcdGetBool (PcdEnableSmmus)) > >>> +#endif > >>> + { > >>> DisableSmmu (Fdt, "iommus", "/smb/smmu@e0600000", "/smb/xgmac@e0700000"); > >>> DisableSmmu (Fdt, "iommus", "/smb/smmu@e0800000", "/smb/xgmac@e0900000"); > >>> -#endif > >>> } > >> > >> That's a little bit filthy. > >> Could you do > >> > >> #if !DO_XBGE > >> > >> Around the whole new if-statement instead? > >> > > > > Not really. What I actually need is > > > > if (!PcdGetBool (PcdEnableSmmus) || !DO_XGBE) > > > > which is not equivalent. However, this breaks due to the fact the > > DO_XGBE is not defined to 0 but simply not defined at all. > > How about > > #if DO_XGBE > DisableXgbeSmmus = !PcdGetBool (PcdEnableSmmus); > #else > DisableXgbeSmmus = TRUE; > #endif > > if (DisableXgbeSmmus) { > DisableSmmu (Fdt, "iommus", "/smb/smmu@e0600000", "/smb/xgmac@e0700000"); > DisableSmmu (Fdt, "iommus", "/smb/smmu@e0800000", "/smb/xgmac@e0900000"); > } > > ? That looks optimal. With that: Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
diff --git a/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c b/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c index 093db6517c1a..0da00655396e 100644 --- a/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c +++ b/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c @@ -261,11 +261,18 @@ SetSocIdStatus ( if (!PcdGetBool (PcdEnableSmmus)) { DisableSmmu (Fdt, "iommu-map", "/smb/smmu@e0a00000", "/smb/pcie@f0000000"); DisableSmmu (Fdt, "iommus", "/smb/smmu@e0200000", "/smb/sata@e0300000"); + } + + if (!PcdGetBool (PcdEnableSmmus) || !IsRevB1 || FixedPcdGet8 (PcdSata1PortCount) == 0) { DisableSmmu (Fdt, "iommus", "/smb/smmu@e0c00000", "/smb/sata@e0d00000"); + } + #if DO_XGBE + if (!PcdGetBool (PcdEnableSmmus)) +#endif + { DisableSmmu (Fdt, "iommus", "/smb/smmu@e0600000", "/smb/xgmac@e0700000"); DisableSmmu (Fdt, "iommus", "/smb/smmu@e0800000", "/smb/xgmac@e0900000"); -#endif } }
The logic regarding how the various SMMUs are exposed in the device tree is inverted, in the sense that they are present in the static DTB image, and are removed if no SMMU support is requested. However, the logic is flawed in the sense that it did not remove SMMUs for hardware that is not there to begin with, i.e., the XGBE network ports on Cello/Softiron 1000 or the second SATA controller on B1 silicon. So fix that. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)