| Message ID | CACPK8XdHH1Yrr+TaD-joq0uFgj+epg_dQu2QHhSfFv9i0LNkDQ@mail.gmail.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Tue, Feb 28, 2017, at 20:10, Joel Stanley wrote: > > > On 28 Feb 2017 17:49, "Lei YU" <mine260309@gmail.com> wrote: >> The PNOR SPI address mapping is the same as Witherspoon. > > This should be handled by the device drivers we now have. > > Mbox brains trust, any idea why we would still need this? I was going to say NAK for the same reason. Lei: are you absolutely positive you need this? If you do something has gone wrong. Is mboxd integrated into the romulus image? If not we need a yocto patch rather than a kernel patch. Andrew > Cheers, > > Joel > >> >> Signed-off-by: Lei YU <mine260309@gmail.com> >> --- >> arch/arm/mach-aspeed/aspeed.c | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/arch/arm/mach-aspeed/aspeed.c b/arch/arm/mach- >> aspeed/aspeed.c >> index 726b8fa..ec9eecf 100644 >> --- a/arch/arm/mach-aspeed/aspeed.c >> +++ b/arch/arm/mach-aspeed/aspeed.c >> @@ -221,6 +221,24 @@ static void __init do_witherspoon_setup(void) >> static void __init do_romulus_setup(void) >> { >> do_common_setup(); >> + >> + /* Setup PNOR address mapping for 64M flash >> + * >> + * ADRBASE: 0x3000 (0x30000000) >> + * HWMBASE: 0x0C00 (0x0C000000) >> + * ADDRMASK: 0xFC00 (0xFC000000) >> + * HWNCARE: 0x03FF (0x03FF0000) >> + * >> + * Mapping appears at 0x60300fc000000 on the host >> + */ >> + writel(0x30000C00, AST_IO(AST_BASE_LPC | 0x88)); >> + writel(0xFC0003FF, AST_IO(AST_BASE_LPC | 0x8C)); >> + >> + /* Set SPI1 CE1 decoding window to 0x34000000 */ >> + writel(0x70680000, AST_IO(AST_BASE_SPI | 0x34)); >> + >> + /* Set SPI1 CE0 decoding window to 0x30000000 */ >> + writel(0x68600000, AST_IO(AST_BASE_SPI | 0x30)); >> } >> >> #define SCU_PASSWORD 0x1688A8A8 >> -- >> 1.9.1
Hi Andrew, Joel, This patch is for issue https://github.com/openbmc/openbmc/issues/1214 Here's what I have tested: 1. rom1-19 with updated OpenBMC (c85f23e1), which has mboxd enabled; 2. Do a power cycle; 3. Once BMC is ready, check the registers by devmem: ``` root@romulus:/tmp# devmem 0x1e630034 0x70640000 <== Wrong, expect 0x70680000 root@romulus:/tmp# devmem 0x1e630030 0x64600000 <== Wrong, expect 0x68600000 ``` I can see the two registers have the unexpected value. Without manually setting the two registers, `obmcutil poweron` results in no output at all in host console. That's why I send this patch to initialize the registers. Note: I think the lines + writel(0x30000C00, AST_IO(AST_BASE_LPC | 0x88)); + writel(0xFC0003FF, AST_IO(AST_BASE_LPC | 0x8C)); are not necessary since they are having the expected value without this patch. But below changes are necessary: + /* Set SPI1 CE1 decoding window to 0x34000000 */ + writel(0x70680000, AST_IO(AST_BASE_SPI | 0x34)); + + /* Set SPI1 CE0 decoding window to 0x30000000 */ + writel(0x68600000, AST_IO(AST_BASE_SPI | 0x30)); -- BRs, Lei YU On Tue, Feb 28, 2017 at 7:16 PM, Andrew Jeffery <andrew@aj.id.au> wrote: > On Tue, Feb 28, 2017, at 20:10, Joel Stanley wrote: > > > > On 28 Feb 2017 17:49, "Lei YU" <mine260309@gmail.com> wrote: > > The PNOR SPI address mapping is the same as Witherspoon. > > > This should be handled by the device drivers we now have. > > Mbox brains trust, any idea why we would still need this? > > > I was going to say NAK for the same reason. > > Lei: are you absolutely positive you need this? If you do something has gone > wrong. Is mboxd integrated into the romulus image? If not we need a yocto > patch rather than a kernel patch. > > Andrew > > Cheers, > > Joel > > > Signed-off-by: Lei YU <mine260309@gmail.com> > --- > arch/arm/mach-aspeed/aspeed.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/arch/arm/mach-aspeed/aspeed.c b/arch/arm/mach-aspeed/aspeed.c > index 726b8fa..ec9eecf 100644 > --- a/arch/arm/mach-aspeed/aspeed.c > +++ b/arch/arm/mach-aspeed/aspeed.c > @@ -221,6 +221,24 @@ static void __init do_witherspoon_setup(void) > static void __init do_romulus_setup(void) > { > do_common_setup(); > + > + /* Setup PNOR address mapping for 64M flash > + * > + * ADRBASE: 0x3000 (0x30000000) > + * HWMBASE: 0x0C00 (0x0C000000) > + * ADDRMASK: 0xFC00 (0xFC000000) > + * HWNCARE: 0x03FF (0x03FF0000) > + * > + * Mapping appears at 0x60300fc000000 on the host > + */ > + writel(0x30000C00, AST_IO(AST_BASE_LPC | 0x88)); > + writel(0xFC0003FF, AST_IO(AST_BASE_LPC | 0x8C)); > + > + /* Set SPI1 CE1 decoding window to 0x34000000 */ > + writel(0x70680000, AST_IO(AST_BASE_SPI | 0x34)); > + > + /* Set SPI1 CE0 decoding window to 0x30000000 */ > + writel(0x68600000, AST_IO(AST_BASE_SPI | 0x30)); > } > > #define SCU_PASSWORD 0x1688A8A8 > -- > 1.9.1
On Tue, 2017-02-28 at 20:27 +0800, Lei YU wrote: > Hi Andrew, Joel, > > This patch is for issue https://github.com/openbmc/openbmc/issues/1214 > > Here's what I have tested: > 1. rom1-19 with updated OpenBMC (c85f23e1), which has mboxd enabled; > 2. Do a power cycle; > 3. Once BMC is ready, check the registers by devmem: > ``` > > root@romulus:/tmp# devmem 0x1e630034 > 0x70640000 <== Wrong, expect 0x70680000 > > root@romulus:/tmp# devmem 0x1e630030 > 0x64600000 <== Wrong, expect 0x68600000 > ``` > I can see the two registers have the unexpected value. > > Without manually setting the two registers, `obmcutil poweron` results in > no output at all in host console. Okay, can you please open an issue on github? I'd like to have a play with a Romulus system tomorrow to better understand what's going on here. We shouldn't need this kernel patch. This makes me wonder if something's wrong with Zaius and Witherspoon as well, and whether the problem is masked by these register writes in the boardfile. Thanks for the detailed reply. Andrew > > That's why I send this patch to initialize the registers. > > Note: I think the lines > + writel(0x30000C00, AST_IO(AST_BASE_LPC | 0x88)); > + writel(0xFC0003FF, AST_IO(AST_BASE_LPC | 0x8C)); > are not necessary since they are having the expected value without this patch. > > But below changes are necessary: > + /* Set SPI1 CE1 decoding window to 0x34000000 */ > + writel(0x70680000, AST_IO(AST_BASE_SPI | 0x34)); > + > + /* Set SPI1 CE0 decoding window to 0x30000000 */ > + writel(0x68600000, AST_IO(AST_BASE_SPI | 0x30)); > > -- > BRs, > Lei YU > > > On Tue, Feb 28, 2017 at 7:16 PM, Andrew Jeffery <andrew@aj.id.au> wrote: > > On Tue, Feb 28, 2017, at 20:10, Joel Stanley wrote: > > > > > > > > > > On 28 Feb 2017 17:49, "Lei YU" <mine260309@gmail.com> wrote: > > > > The PNOR SPI address mapping is the same as Witherspoon. > > > > > > This should be handled by the device drivers we now have. > > > > Mbox brains trust, any idea why we would still need this? > > > > > > I was going to say NAK for the same reason. > > > > Lei: are you absolutely positive you need this? If you do something has gone > > wrong. Is mboxd integrated into the romulus image? If not we need a yocto > > patch rather than a kernel patch. > > > > Andrew > > > > Cheers, > > > > Joel > > > > > > > > Signed-off-by: Lei YU <mine260309@gmail.com> > > --- > > arch/arm/mach-aspeed/aspeed.c | 18 ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > > diff --git a/arch/arm/mach-aspeed/aspeed.c b/arch/arm/mach-aspeed/aspeed.c > > index 726b8fa..ec9eecf 100644 > > --- a/arch/arm/mach-aspeed/aspeed.c > > +++ b/arch/arm/mach-aspeed/aspeed.c > > @@ -221,6 +221,24 @@ static void __init do_witherspoon_setup(void) > > static void __init do_romulus_setup(void) > > { > > do_common_setup(); > > + > > + /* Setup PNOR address mapping for 64M flash > > + * > > + * ADRBASE: 0x3000 (0x30000000) > > + * HWMBASE: 0x0C00 (0x0C000000) > > + * ADDRMASK: 0xFC00 (0xFC000000) > > + * HWNCARE: 0x03FF (0x03FF0000) > > + * > > + * Mapping appears at 0x60300fc000000 on the host > > + */ > > + writel(0x30000C00, AST_IO(AST_BASE_LPC | 0x88)); > > + writel(0xFC0003FF, AST_IO(AST_BASE_LPC | 0x8C)); > > + > > + /* Set SPI1 CE1 decoding window to 0x34000000 */ > > + writel(0x70680000, AST_IO(AST_BASE_SPI | 0x34)); > > + > > + /* Set SPI1 CE0 decoding window to 0x30000000 */ > > + writel(0x68600000, AST_IO(AST_BASE_SPI | 0x30)); > > } > > > > #define SCU_PASSWORD 0x1688A8A8 > > -- > > 1.9.1
On Tue, 2017-02-28 at 23:10 +1030, Andrew Jeffery wrote: > > This patch is for issue https://github.com/openbmc/openbmc/issues/1214 ... > Okay, can you please open an issue on github? Sorry, I completely looked past that link the first time. Andrew
Replying to myself again... I think I've learnt the lesson not to email whilst in bed. I've done some further investigation after jumping on a Romulus machine: On Tue, 2017-02-28 at 23:10 +1030, Andrew Jeffery wrote: > On Tue, 2017-02-28 at 20:27 +0800, Lei YU wrote: > > Hi Andrew, Joel, > > > > This patch is for issue https://github.com/openbmc/openbmc/issues/1214 > > > > Here's what I have tested: > > 1. rom1-19 with updated OpenBMC (c85f23e1), which has mboxd enabled; > > 2. Do a power cycle; > > 3. Once BMC is ready, check the registers by devmem: > > ``` > > > root@romulus:/tmp# devmem 0x1e630034 > > > > 0x70640000 <== Wrong, expect 0x70680000 > > > root@romulus:/tmp# devmem 0x1e630030 > > > > 0x64600000 <== Wrong, expect 0x68600000 > > ``` > > I can see the two registers have the unexpected value. > > > > Without manually setting the two registers, `obmcutil poweron` results in > > no output at all in host console. > > Okay, can you please open an issue on github? I'd like to have a play > with a Romulus system tomorrow to better understand what's going on > here. We shouldn't need this kernel patch. > > This makes me wonder if something's wrong with Zaius and Witherspoon as > well, and whether the problem is masked by these register writes in the > boardfile. > I had assumed here you were talking about the HICR{7,8} registers, and if I was paying more attention I would have realised you are not. Instead, these are the {S,F}MC segment mapping configuration registers. The default values correspond to a 32MB part. From the datasheet if we do the transform ((0x64600000 >> 16) >> 1) we get 0x3230 which translates to "start the mapping at 0x30000000, end at 0x32000000" and thus a 32MiB mapping. With your expected values: ((0x68600000 >> 16) >> 1) we get 0x3430, or "start the mapping at 0x30000000, end at 0x34000000", which is a 64MiB mapping. Looking at the driver[1] the only time we touch the segment register is to read the window start, we don't ever write the segment mapping configuration. Cédric: Have you looked into supporting configuration of the segment mapping? We could configure the mapping if we know the chip sizes, however looking at the device's bindings documentation[2] we find: ... Required properties: ... - #size-cells : must be 0 corresponding to chip select child binding and ... Child node required properties: - reg : must contain chip select number in first cell of address, must be 1 tuple long ... I think we need #size-cells to be 1 in the parent so we can define the mapping ranges. I don't know that its going to be an easy change to make though given the bindings are already upstream. Thoughts? Lei: So as it stands you should drop the HICRx writes from your patch. However as it stands, we still need the writes to the SMC segment registers until we can sort out proper driver support. Cheers, Andrew [1] https://github.com/openbmc/linux/blob/dev-4.7/drivers/mtd/spi-nor/aspeed-smc.c#L728 [2] https://github.com/openbmc/linux/blob/dev-4.7/Documentation/devicetree/bindings/mtd/aspeed-smc.txt
On 03/01/2017 04:05 AM, Andrew Jeffery wrote: > Replying to myself again... I think I've learnt the lesson not to email > whilst in bed. > > I've done some further investigation after jumping on a Romulus > machine: > > On Tue, 2017-02-28 at 23:10 +1030, Andrew Jeffery wrote: >> On Tue, 2017-02-28 at 20:27 +0800, Lei YU wrote: >>> Hi Andrew, Joel, >>> >>> This patch is for issue https://github.com/openbmc/openbmc/issues/1214 >>> >>> Here's what I have tested: >>> 1. rom1-19 with updated OpenBMC (c85f23e1), which has mboxd enabled; >>> 2. Do a power cycle; >>> 3. Once BMC is ready, check the registers by devmem: >>> ``` >>>> root@romulus:/tmp# devmem 0x1e630034 >>> >>> 0x70640000 <== Wrong, expect 0x70680000 >>>> root@romulus:/tmp# devmem 0x1e630030 >>> >>> 0x64600000 <== Wrong, expect 0x68600000 >>> ``` >>> I can see the two registers have the unexpected value. These are the default values : SPIR30: SPI1 CE0 Address Decoding Range Register Init = 0x6460 0000 SPIR34: SPI1 CE1 Address Decoding Range Register Init = 0x7064 0000 We should let the driver configure these values I think. see below. >>> Without manually setting the two registers, `obmcutil poweron` results in >>> no output at all in host console. >> >> Okay, can you please open an issue on github? I'd like to have a play >> with a Romulus system tomorrow to better understand what's going on >> here. We shouldn't need this kernel patch. >> >> This makes me wonder if something's wrong with Zaius and Witherspoon as >> well, and whether the problem is masked by these register writes in the >> boardfile. >> > > I had assumed here you were talking about the HICR{7,8} registers, and > if I was paying more attention I would have realised you are not. > > Instead, these are the {S,F}MC segment mapping configuration registers. > The default values correspond to a 32MB part. All the defaults value can be found in the QEMU Aspeed SMC model also : https://github.com/openbmc/qemu/blob/master/hw/ssi/aspeed_smc.c#L174 It might be a little easier to find. > From the datasheet if we > do the transform ((0x64600000 >> 16) >> 1) we get 0x3230 which > translates to "start the mapping at 0x30000000, end at 0x32000000" and You can multiply 0x64 by 8MB, which would give the same results. > thus a 32MiB mapping. With your expected values: ((0x68600000 >> 16) >> > 1) we get 0x3430, or "start the mapping at 0x30000000, end at > 0x34000000", which is a 64MiB mapping. > > Looking at the driver[1] the only time we touch the segment register is > to read the window start, we don't ever write the segment mapping > configuration. yes, not in 4.7 yet. It is a little "complex" to configure the segments up to 5 chips handling the possible overlaps, the different chip sizes, which might be bigger than the maximum window size, and the HW bugs. AST2500 SPI controller has a bug when the CE0 chip size exceeds 120MB. Also the controller only really makes use of them in command mode. In user mode, the start address is mostly used to identify the chip when writing commands on the AHB Bus. > Cédric: Have you looked into supporting configuration of the segment > mapping? yes. QEMU (openbmc and mainline) supports them already. As for the kernel, here is an initial commit : https://github.com/legoater/linux/commit/c08765882b4091cb1b9998d872415c072ecfe8af which I will send with a bunch of improvements when we switch to linux 4.10. I can eventually rebase on 4.7 but I would have preferred not maintaining too much branches. > We could configure the mapping if we know the chip sizes, however > looking at the device's bindings documentation[2] we find: > > ... > Required properties: > ... > - #size-cells : must be 0 corresponding to chip select child binding > > and > > ... > Child node required properties: > - reg : must contain chip select number in first cell of address, must > be 1 tuple long > ... > > I think we need #size-cells to be 1 in the parent so we can define the > mapping ranges. I don't know that its going to be an easy change to > make though given the bindings are already upstream. Thoughts? In the aspeed-smc bindings, the mapping range is for the overall AHB window. This is a HW constant you can not change. As for the settings of the each chip window, I would let the driver discover and configure them as you can ruin the controller behavior in command mode if they are wrong. What is the specific need ? Is it to have a window covering all the SPI CEx chips ? If so, my tree should have that support already. But, please note that the HW is broken for chips > 128MB, on witherspoon for instance. Cheers, C. > Lei: So as it stands you should drop the HICRx writes from your patch. > However as it stands, we still need the writes to the SMC segment > registers until we can sort out proper driver support. > > Cheers, > > Andrew > > [1] https://github.com/openbmc/linux/blob/dev-4.7/drivers/mtd/spi-nor/aspeed-smc.c#L728 > [2] https://github.com/openbmc/linux/blob/dev-4.7/Documentation/devicetree/bindings/mtd/aspeed-smc.txt >
On Wed, 2017-03-01 at 08:26 +0100, Cédric Le Goater wrote: > On 03/01/2017 04:05 AM, Andrew Jeffery wrote: > > Replying to myself again... I think I've learnt the lesson not to email > > whilst in bed. > > > > I've done some further investigation after jumping on a Romulus > > machine: > > > > On Tue, 2017-02-28 at 23:10 +1030, Andrew Jeffery wrote: > > > On Tue, 2017-02-28 at 20:27 +0800, Lei YU wrote: > > > > Hi Andrew, Joel, > > > > > > > > This patch is for issue https://github.com/openbmc/openbmc/issues/1214 > > > > > > > > Here's what I have tested: > > > > 1. rom1-19 with updated OpenBMC (c85f23e1), which has mboxd enabled; > > > > 2. Do a power cycle; > > > > 3. Once BMC is ready, check the registers by devmem: > > > > ``` > > > > > root@romulus:/tmp# devmem 0x1e630034 > > > > > > > > 0x70640000 <== Wrong, expect 0x70680000 > > > > > root@romulus:/tmp# devmem 0x1e630030 > > > > > > > > 0x64600000 <== Wrong, expect 0x68600000 > > > > ``` > > > > I can see the two registers have the unexpected value. > > These are the default values : > > SPIR30: SPI1 CE0 Address Decoding Range Register Init = 0x6460 0000 > SPIR34: SPI1 CE1 Address Decoding Range Register Init = 0x7064 0000 > > We should let the driver configure these values I think. > see below. > Agreed. > > > > Looking at the driver[1] the only time we touch the segment register is > > to read the window start, we don't ever write the segment mapping > > configuration. > > yes, not in 4.7 yet. It is a little "complex" to configure the > segments up to 5 chips handling the possible overlaps, the > different chip sizes, which might be bigger than the maximum > window size, and the HW bugs. AST2500 SPI controller has a bug > when the CE0 chip size exceeds 120MB. Yeah, I appreciate it's a finicky problem. > > Also the controller only really makes use of them in command > mode. In user mode, the start address is mostly used to > identify the chip when writing commands on the AHB Bus. > > > Cédric: Have you looked into supporting configuration of the segment > > mapping? > > yes. QEMU (openbmc and mainline) supports them already. > > As for the kernel, here is an initial commit : > > https://github.com/legoater/linux/commit/c08765882b4091cb1b9998d872415c072ecfe8af > > which I will send with a bunch of improvements when we switch > to linux 4.10. I can eventually rebase on 4.7 but I would have > preferred not maintaining too much branches. That's fair enough. If we apply Lei's patch to 4.7 as a hack it means that we can ignore the issue from then on. Any new systems will be based on at least 4.10, which should have your improvements. > > > We could configure the mapping if we know the chip sizes, however > > looking at the device's bindings documentation[2] we find: > > > > ... > > Required properties: > > ... > > - #size-cells : must be 0 corresponding to chip select child binding > > > > and > > > > ... > > Child node required properties: > > - reg : must contain chip select number in first cell of address, must > > be 1 tuple long > > ... > > > > I think we need #size-cells to be 1 in the parent so we can define the > > mapping ranges. I don't know that its going to be an easy change to > > make though given the bindings are already upstream. Thoughts? > > In the aspeed-smc bindings, the mapping range is for the overall > AHB window. This is a HW constant you can not change. > > As for the settings of the each chip window, I would let the > driver discover and configure them as you can ruin the controller > behavior in command mode if they are wrong. I wasn't concerned about the overall AHB window so much as the chip windows. Letting the driver discover and configure them was what I was hoping for, and was investigating providing the information via the DT. If we have an alternative approach (querying the chip?) that would be great. > > What is the specific need ? Is it to have a window covering all > the SPI CEx chips ? In this case it was to have CE0 window cover 64MB rather than 32MB. Supporting more than one chip would be a nice-to-have rather than something Lei needs as far as I understand. > If so, my tree should have that support > already. But, please note that the HW is broken for chips > 128MB, > on witherspoon for instance. > I guess the question is about what support is going to be in which branch of openbmc/linux. As it stands it looks like dev-4.7 will not have chip window configuration support in the driver, and dev-4.10 will. Andrew
On 03/02/2017 03:55 AM, Andrew Jeffery wrote: > On Wed, 2017-03-01 at 08:26 +0100, Cédric Le Goater wrote: >> On 03/01/2017 04:05 AM, Andrew Jeffery wrote: >>> Replying to myself again... I think I've learnt the lesson not to email >>> whilst in bed. >>> >>> I've done some further investigation after jumping on a Romulus >>> machine: >>> >>> On Tue, 2017-02-28 at 23:10 +1030, Andrew Jeffery wrote: >>>> On Tue, 2017-02-28 at 20:27 +0800, Lei YU wrote: >>>>> Hi Andrew, Joel, >>>>> >>>>> This patch is for issue https://github.com/openbmc/openbmc/issues/1214 >>>>> >>>>> Here's what I have tested: >>>>> 1. rom1-19 with updated OpenBMC (c85f23e1), which has mboxd enabled; >>>>> 2. Do a power cycle; >>>>> 3. Once BMC is ready, check the registers by devmem: >>>>> ``` >>>>>> root@romulus:/tmp# devmem 0x1e630034 >>>>> >>>>> 0x70640000 <== Wrong, expect 0x70680000 >>>>>> root@romulus:/tmp# devmem 0x1e630030 >>>>> >>>>> 0x64600000 <== Wrong, expect 0x68600000 >>>>> ``` >>>>> I can see the two registers have the unexpected value. >> >> These are the default values : >> >> SPIR30: SPI1 CE0 Address Decoding Range Register Init = 0x6460 0000 >> SPIR34: SPI1 CE1 Address Decoding Range Register Init = 0x7064 0000 >> >> We should let the driver configure these values I think. >> see below. >> > > Agreed. > >>> >>> Looking at the driver[1] the only time we touch the segment register is >>> to read the window start, we don't ever write the segment mapping >>> configuration. >> >> yes, not in 4.7 yet. It is a little "complex" to configure the >> segments up to 5 chips handling the possible overlaps, the >> different chip sizes, which might be bigger than the maximum >> window size, and the HW bugs. AST2500 SPI controller has a bug >> when the CE0 chip size exceeds 120MB. > > Yeah, I appreciate it's a finicky problem. > >> >> Also the controller only really makes use of them in command >> mode. In user mode, the start address is mostly used to >> identify the chip when writing commands on the AHB Bus. >> >>> Cédric: Have you looked into supporting configuration of the segment >>> mapping? >> >> yes. QEMU (openbmc and mainline) supports them already. >> >> As for the kernel, here is an initial commit : >> >> https://github.com/legoater/linux/commit/c08765882b4091cb1b9998d872415c072ecfe8af >> >> which I will send with a bunch of improvements when we switch >> to linux 4.10. I can eventually rebase on 4.7 but I would have >> preferred not maintaining too much branches. > > That's fair enough. If we apply Lei's patch to 4.7 as a hack it means > that we can ignore the issue from then on. Any new systems will be > based on at least 4.10, which should have your improvements. > >> >>> We could configure the mapping if we know the chip sizes, however >>> looking at the device's bindings documentation[2] we find: >>> >>> ... >>> Required properties: >>> ... >>> - #size-cells : must be 0 corresponding to chip select child binding >>> >>> and >>> >>> ... >>> Child node required properties: >>> - reg : must contain chip select number in first cell of address, must >>> be 1 tuple long >>> ... >>> >>> I think we need #size-cells to be 1 in the parent so we can define the >>> mapping ranges. I don't know that its going to be an easy change to >>> make though given the bindings are already upstream. Thoughts? >> >> In the aspeed-smc bindings, the mapping range is for the overall >> AHB window. This is a HW constant you can not change. >> >> As for the settings of the each chip window, I would let the >> driver discover and configure them as you can ruin the controller >> behavior in command mode if they are wrong. > > I wasn't concerned about the overall AHB window so much as the chip > windows. Letting the driver discover and configure them was what I was > hoping for, and was investigating providing the information via the DT. Well, we possibly could but given that we can change the chip (and possibly their size), it is better to try to detect them I think. > If we have an alternative approach (querying the chip?) that would be > great. That is what my branch is trying to do but it still needs a couple of improvements. All segments should be configured, even those for which there are no chips. Lei just reminded me of that problem. >> What is the specific need ? Is it to have a window covering all >> the SPI CEx chips ? > > In this case it was to have CE0 window cover 64MB rather than 32MB. > Supporting more than one chip would be a nice-to-have rather than > something Lei needs as far as I understand. There is still that segment overlap problem to cover even if there are no chips. >> If so, my tree should have that support >> already. But, please note that the HW is broken for chips > 128MB, >> on witherspoon for instance. >> > > I guess the question is about what support is going to be in which > branch of openbmc/linux. As it stands it looks like dev-4.7 will not > have chip window configuration support in the driver, and dev-4.10 > will. Unless we start using what's in my branch which has a backport of the 4.11 driver on openbmc 4.7. It should be fine I think. I just need to cover the above case, moving the start address of the next segment should be it, plus a couple of checks. Cheers, C.
diff --git a/arch/arm/mach-aspeed/aspeed.c b/arch/arm/mach-aspeed/aspeed.c index 726b8fa..ec9eecf 100644 --- a/arch/arm/mach-aspeed/aspeed.c +++ b/arch/arm/mach-aspeed/aspeed.c @@ -221,6 +221,24 @@ static void __init do_witherspoon_setup(void) static void __init do_romulus_setup(void) { do_common_setup(); + + /* Setup PNOR address mapping for 64M flash + * + * ADRBASE: 0x3000 (0x30000000) + * HWMBASE: 0x0C00 (0x0C000000) + * ADDRMASK: 0xFC00 (0xFC000000) + * HWNCARE: 0x03FF (0x03FF0000) + * + * Mapping appears at 0x60300fc000000 on the host + */ + writel(0x30000C00, AST_IO(AST_BASE_LPC | 0x88)); + writel(0xFC0003FF, AST_IO(AST_BASE_LPC | 0x8C)); + + /* Set SPI1 CE1 decoding window to 0x34000000 */ + writel(0x70680000, AST_IO(AST_BASE_SPI | 0x34)); + + /* Set SPI1 CE0 decoding window to 0x30000000 */ + writel(0x68600000, AST_IO(AST_BASE_SPI | 0x30)); } #define SCU_PASSWORD 0x1688A8A8
On 28 Feb 2017 17:49, "Lei YU" <mine260309@gmail.com> wrote: The PNOR SPI address mapping is the same as Witherspoon. This should be handled by the device drivers we now have. Mbox brains trust, any idea why we would still need this? Cheers, Joel Signed-off-by: Lei YU <mine260309@gmail.com> --- arch/arm/mach-aspeed/aspeed.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) -- 1.9.1