| Message ID | 20201108230232.1044461-1-pbrobinson@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | rockchip: Move Bob specific bits to it's specific u-boot.dtsi | expand |
Hi Peter, On 2020/11/9 上午7:02, Peter Robinson wrote: > Move the bits that are device specific to the -u-boot.dtsi as the > bits may be different on other devices and hence breaks SPI on > those devices such as the Pinebook Pro. > > Signed-off-by: Peter Robinson <pbrobinson@gmail.com> > Fixes: c4cea2bbf995 ("rockchip: Enable building a SPI ROM image on bob") > Cc: Simon Glass <sjg@chromium.org> > --- > arch/arm/dts/rk3399-gru-u-boot.dtsi | 30 +++++++++++++++++++++++++++++ > arch/arm/dts/rk3399-u-boot.dtsi | 25 ------------------------ > 2 files changed, 30 insertions(+), 25 deletions(-) > > diff --git a/arch/arm/dts/rk3399-gru-u-boot.dtsi b/arch/arm/dts/rk3399-gru-u-boot.dtsi > index 390ac2bb5a..5e95cacfea 100644 > --- a/arch/arm/dts/rk3399-gru-u-boot.dtsi > +++ b/arch/arm/dts/rk3399-gru-u-boot.dtsi > @@ -5,6 +5,36 @@ > > #include "rk3399-u-boot.dtsi" > > +/ { > + aliases { > + spi1 = &spi1; > + }; > +}; Does this still need to remove from common code after your another patch applied? It look reasonable and not likely to break others. https://patchwork.ozlabs.org/project/uboot/patch/20201108140023.32501-1-sigmaris@gmail.com/ > + > +#ifdef CONFIG_ROCKCHIP_SPI_IMAGE > +&binman { > + rom { > + filename = "u-boot.rom"; > + size = <0x400000>; > + pad-byte = <0xff>; > + > + mkimage { > + args = "-n rk3399 -T rkspi"; > + u-boot-spl { > + }; > + }; > + u-boot-img { > + offset = <0x40000>; > + }; > + u-boot { > + offset = <0x300000>; > + }; > + fdtmap { > + }; > + }; > +}; > +#endif What's the image space mapping for Pinebook Pro do you using? I think there should be another binman config if this is not common . Thanks, - Kever > + > &spi_flash { > u-boot,dm-pre-reloc; > }; > diff --git a/arch/arm/dts/rk3399-u-boot.dtsi b/arch/arm/dts/rk3399-u-boot.dtsi > index ecd230c720..26b0a34e64 100644 > --- a/arch/arm/dts/rk3399-u-boot.dtsi > +++ b/arch/arm/dts/rk3399-u-boot.dtsi > @@ -11,7 +11,6 @@ > mmc0 = &sdhci; > mmc1 = &sdmmc; > pci0 = &pcie0; > - spi1 = &spi1; > }; > > cic: syscon@ff620000 { > @@ -60,30 +59,6 @@ > > }; > > -#ifdef CONFIG_ROCKCHIP_SPI_IMAGE > -&binman { > - rom { > - filename = "u-boot.rom"; > - size = <0x400000>; > - pad-byte = <0xff>; > - > - mkimage { > - args = "-n rk3399 -T rkspi"; > - u-boot-spl { > - }; > - }; > - u-boot-img { > - offset = <0x40000>; > - }; > - u-boot { > - offset = <0x300000>; > - }; > - fdtmap { > - }; > - }; > -}; > -#endif > - > &cru { > u-boot,dm-pre-reloc; > };
Hi, > On 10 Nov 2020, at 07:34, Kever Yang <kever.yang@rock-chips.com> wrote: > > Hi Peter, > > On 2020/11/9 上午7:02, Peter Robinson wrote: >> Move the bits that are device specific to the -u-boot.dtsi as the >> bits may be different on other devices and hence breaks SPI on >> those devices such as the Pinebook Pro. >> >> Signed-off-by: Peter Robinson <pbrobinson@gmail.com> >> Fixes: c4cea2bbf995 ("rockchip: Enable building a SPI ROM image on bob") >> Cc: Simon Glass <sjg@chromium.org> >> --- >> arch/arm/dts/rk3399-gru-u-boot.dtsi | 30 +++++++++++++++++++++++++++++ >> arch/arm/dts/rk3399-u-boot.dtsi | 25 ------------------------ >> 2 files changed, 30 insertions(+), 25 deletions(-) >> >> diff --git a/arch/arm/dts/rk3399-gru-u-boot.dtsi b/arch/arm/dts/rk3399-gru-u-boot.dtsi >> index 390ac2bb5a..5e95cacfea 100644 >> --- a/arch/arm/dts/rk3399-gru-u-boot.dtsi >> +++ b/arch/arm/dts/rk3399-gru-u-boot.dtsi >> @@ -5,6 +5,36 @@ >> #include "rk3399-u-boot.dtsi" >> +/ { >> + aliases { >> + spi1 = &spi1; >> + }; >> +}; > > Does this still need to remove from common code after your another patch applied? It look reasonable and > > not likely to break others. > > https://patchwork.ozlabs.org/project/uboot/patch/20201108140023.32501-1-sigmaris@gmail.com/ My patch linked above was aiming to fix SPI boot on the rockpro64 by adapting to the spi1 alias that's now in the rk3399-u-boot.dtsi rather than removing it (in fact, my patch wouldn't work correctly if the spi1 alias was removed). This seemed like one good solution as the RK3399 does physically have SPI buses 0 to 5, and out of those the SPI flash is on bus 1, so I thought it'd be better to refer to it as bus 1 instead of aliasing it to bus 0. I see now, though, that it's not just the rockpro64 that's affected, but also Pinebook Pro and it seems rk3399-roc-pc. If the consensus is that my patch is the right approach, I can send another series with the same type of fix for the PBP and rk3399-roc-pc. Otherwise this patch should be used and my patch shouldn't be applied as it relies on the spi1 alias that this patch removes. Regards, Hugh > >> + >> +#ifdef CONFIG_ROCKCHIP_SPI_IMAGE >> +&binman { >> + rom { >> + filename = "u-boot.rom"; >> + size = <0x400000>; >> + pad-byte = <0xff>; >> + >> + mkimage { >> + args = "-n rk3399 -T rkspi"; >> + u-boot-spl { >> + }; >> + }; >> + u-boot-img { >> + offset = <0x40000>; >> + }; >> + u-boot { >> + offset = <0x300000>; >> + }; >> + fdtmap { >> + }; >> + }; >> +}; >> +#endif > > > What's the image space mapping for Pinebook Pro do you using? > > I think there should be another binman config if this is not common . > > > Thanks, > > - Kever > >> + >> &spi_flash { >> u-boot,dm-pre-reloc; >> }; >> diff --git a/arch/arm/dts/rk3399-u-boot.dtsi b/arch/arm/dts/rk3399-u-boot.dtsi >> index ecd230c720..26b0a34e64 100644 >> --- a/arch/arm/dts/rk3399-u-boot.dtsi >> +++ b/arch/arm/dts/rk3399-u-boot.dtsi >> @@ -11,7 +11,6 @@ >> mmc0 = &sdhci; >> mmc1 = &sdmmc; >> pci0 = &pcie0; >> - spi1 = &spi1; >> }; >> cic: syscon@ff620000 { >> @@ -60,30 +59,6 @@ >> }; >> -#ifdef CONFIG_ROCKCHIP_SPI_IMAGE >> -&binman { >> - rom { >> - filename = "u-boot.rom"; >> - size = <0x400000>; >> - pad-byte = <0xff>; >> - >> - mkimage { >> - args = "-n rk3399 -T rkspi"; >> - u-boot-spl { >> - }; >> - }; >> - u-boot-img { >> - offset = <0x40000>; >> - }; >> - u-boot { >> - offset = <0x300000>; >> - }; >> - fdtmap { >> - }; >> - }; >> -}; >> -#endif >> - >> &cru { >> u-boot,dm-pre-reloc; >> };
On 10/11/2020 20:18, Hugh Cole-Baker wrote: > Hi, > >> On 10 Nov 2020, at 07:34, Kever Yang <kever.yang@rock-chips.com> wrote: >> >> Hi Peter, >> >> On 2020/11/9 上午7:02, Peter Robinson wrote: >>> Move the bits that are device specific to the -u-boot.dtsi as the >>> bits may be different on other devices and hence breaks SPI on >>> those devices such as the Pinebook Pro. >>> >>> Signed-off-by: Peter Robinson <pbrobinson@gmail.com> >>> Fixes: c4cea2bbf995 ("rockchip: Enable building a SPI ROM image on bob") >>> Cc: Simon Glass <sjg@chromium.org> >>> --- >>> arch/arm/dts/rk3399-gru-u-boot.dtsi | 30 +++++++++++++++++++++++++++++ >>> arch/arm/dts/rk3399-u-boot.dtsi | 25 ------------------------ >>> 2 files changed, 30 insertions(+), 25 deletions(-) >>> >>> diff --git a/arch/arm/dts/rk3399-gru-u-boot.dtsi b/arch/arm/dts/rk3399-gru-u-boot.dtsi >>> index 390ac2bb5a..5e95cacfea 100644 >>> --- a/arch/arm/dts/rk3399-gru-u-boot.dtsi >>> +++ b/arch/arm/dts/rk3399-gru-u-boot.dtsi >>> @@ -5,6 +5,36 @@ >>> #include "rk3399-u-boot.dtsi" >>> +/ { >>> + aliases { >>> + spi1 = &spi1; >>> + }; >>> +}; >> >> Does this still need to remove from common code after your another patch applied? It look reasonable and >> >> not likely to break others. >> >> https://patchwork.ozlabs.org/project/uboot/patch/20201108140023.32501-1-sigmaris@gmail.com/ > > My patch linked above was aiming to fix SPI boot on the rockpro64 by > adapting to the spi1 alias that's now in the rk3399-u-boot.dtsi rather > than removing it (in fact, my patch wouldn't work correctly if the spi1 > alias was removed). This seemed like one good solution as the RK3399 does > physically have SPI buses 0 to 5, and out of those the SPI flash is on > bus 1, so I thought it'd be better to refer to it as bus 1 instead of > aliasing it to bus 0. It looks like something similar to your patch works on the Pinebook Pro, see this other thread about rk3399 SPI breakage [1] and one recent mail from it [2]. [1] https://lists.denx.de/pipermail/u-boot/2020-November/432046.html [2] https://lists.denx.de/pipermail/u-boot/2020-November/432505.html > I see now, though, that it's not just the rockpro64 that's affected, but > also Pinebook Pro and it seems rk3399-roc-pc. If the consensus is that > my patch is the right approach, I can send another series with the same > type of fix for the PBP and rk3399-roc-pc. Otherwise this patch should > be used and my patch shouldn't be applied as it relies on the spi1 alias > that this patch removes. Looks to me like the full list is (though I don't have those boards): pinebook-pro-rk3399 rk3399-pinebook-pro-u-boot.dtsi puma-rk3399 rk3399-puma-haikou-u-boot.dtsi roc-pc-rk3399 rk3399-roc-pc-u-boot.dtsi roc-pc-mezzanine-rk3399 `` rockpro64-rk3399 rk3399-rockpro64-u-boot.dtsi > Regards, > Hugh > >> >>> + >>> +#ifdef CONFIG_ROCKCHIP_SPI_IMAGE >>> +&binman { >>> + rom { >>> + filename = "u-boot.rom"; >>> + size = <0x400000>; >>> + pad-byte = <0xff>; >>> + >>> + mkimage { >>> + args = "-n rk3399 -T rkspi"; >>> + u-boot-spl { >>> + }; >>> + }; >>> + u-boot-img { >>> + offset = <0x40000>; >>> + }; >>> + u-boot { >>> + offset = <0x300000>; >>> + }; >>> + fdtmap { >>> + }; >>> + }; >>> +}; >>> +#endif >> >> >> What's the image space mapping for Pinebook Pro do you using? >> >> I think there should be another binman config if this is not common . >> >> >> Thanks, >> >> - Kever >> >>> + >>> &spi_flash { >>> u-boot,dm-pre-reloc; >>> }; >>> diff --git a/arch/arm/dts/rk3399-u-boot.dtsi b/arch/arm/dts/rk3399-u-boot.dtsi >>> index ecd230c720..26b0a34e64 100644 >>> --- a/arch/arm/dts/rk3399-u-boot.dtsi >>> +++ b/arch/arm/dts/rk3399-u-boot.dtsi >>> @@ -11,7 +11,6 @@ >>> mmc0 = &sdhci; >>> mmc1 = &sdmmc; >>> pci0 = &pcie0; >>> - spi1 = &spi1; >>> }; >>> cic: syscon@ff620000 { >>> @@ -60,30 +59,6 @@ >>> }; >>> -#ifdef CONFIG_ROCKCHIP_SPI_IMAGE >>> -&binman { >>> - rom { >>> - filename = "u-boot.rom"; >>> - size = <0x400000>; >>> - pad-byte = <0xff>; >>> - >>> - mkimage { >>> - args = "-n rk3399 -T rkspi"; >>> - u-boot-spl { >>> - }; >>> - }; >>> - u-boot-img { >>> - offset = <0x40000>; >>> - }; >>> - u-boot { >>> - offset = <0x300000>; >>> - }; >>> - fdtmap { >>> - }; >>> - }; >>> -}; >>> -#endif >>> - >>> &cru { >>> u-boot,dm-pre-reloc; >>> }; >
diff --git a/arch/arm/dts/rk3399-gru-u-boot.dtsi b/arch/arm/dts/rk3399-gru-u-boot.dtsi index 390ac2bb5a..5e95cacfea 100644 --- a/arch/arm/dts/rk3399-gru-u-boot.dtsi +++ b/arch/arm/dts/rk3399-gru-u-boot.dtsi @@ -5,6 +5,36 @@ #include "rk3399-u-boot.dtsi" +/ { + aliases { + spi1 = &spi1; + }; +}; + +#ifdef CONFIG_ROCKCHIP_SPI_IMAGE +&binman { + rom { + filename = "u-boot.rom"; + size = <0x400000>; + pad-byte = <0xff>; + + mkimage { + args = "-n rk3399 -T rkspi"; + u-boot-spl { + }; + }; + u-boot-img { + offset = <0x40000>; + }; + u-boot { + offset = <0x300000>; + }; + fdtmap { + }; + }; +}; +#endif + &spi_flash { u-boot,dm-pre-reloc; }; diff --git a/arch/arm/dts/rk3399-u-boot.dtsi b/arch/arm/dts/rk3399-u-boot.dtsi index ecd230c720..26b0a34e64 100644 --- a/arch/arm/dts/rk3399-u-boot.dtsi +++ b/arch/arm/dts/rk3399-u-boot.dtsi @@ -11,7 +11,6 @@ mmc0 = &sdhci; mmc1 = &sdmmc; pci0 = &pcie0; - spi1 = &spi1; }; cic: syscon@ff620000 { @@ -60,30 +59,6 @@ }; -#ifdef CONFIG_ROCKCHIP_SPI_IMAGE -&binman { - rom { - filename = "u-boot.rom"; - size = <0x400000>; - pad-byte = <0xff>; - - mkimage { - args = "-n rk3399 -T rkspi"; - u-boot-spl { - }; - }; - u-boot-img { - offset = <0x40000>; - }; - u-boot { - offset = <0x300000>; - }; - fdtmap { - }; - }; -}; -#endif - &cru { u-boot,dm-pre-reloc; };
Move the bits that are device specific to the -u-boot.dtsi as the bits may be different on other devices and hence breaks SPI on those devices such as the Pinebook Pro. Signed-off-by: Peter Robinson <pbrobinson@gmail.com> Fixes: c4cea2bbf995 ("rockchip: Enable building a SPI ROM image on bob") Cc: Simon Glass <sjg@chromium.org> --- arch/arm/dts/rk3399-gru-u-boot.dtsi | 30 +++++++++++++++++++++++++++++ arch/arm/dts/rk3399-u-boot.dtsi | 25 ------------------------ 2 files changed, 30 insertions(+), 25 deletions(-)