Re: [U-Boot] [PATCH] ARM: socfpga: Configure PL310 latencies
On 3/1/19 10:09 AM, Marek Vasut wrote: > On 3/1/19 4:19 PM, Dinh Nguyen wrote: >> >> >> On 3/1/19 3:40 AM, Marek Vasut wrote: >>> On 3/1/19 12:59 AM, Dinh Nguyen wrote: Hi Marek, On 2/19/19 4:01 AM, Simon Goldschmidt wrote: > On Tue, Feb 19, 2019 at 1:44 AM Marek Vasut wrote: >> >> Configure the PL310 tag and data latency registers, which slightly >> improves performance and aligns the behavior with Linux. >> >> Signed-off-by: Marek Vasut >> Cc: Dalon Westergreen >> Cc: Dinh Nguyen >> --- >> arch/arm/mach-socfpga/misc.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c >> index 78fbe28724..1ea4e32c11 100644 >> --- a/arch/arm/mach-socfpga/misc.c >> +++ b/arch/arm/mach-socfpga/misc.c >> @@ -62,6 +62,9 @@ void v7_outer_cache_enable(void) >> /* Disable the L2 cache */ >> clrbits_le32(>pl310_ctrl, L2X0_CTRL_EN); >> >> + writel(0x111, >pl310_tag_latency_ctrl); >> + writel(0x121, >pl310_data_latency_ctrl); > > Would it make sense to add defines as named constants for this? > OTOH, in Linux, the values in the devicetree aren't really described, > either, so: I was thinking the same, so I'm working on a patch to get these values from the device tree. So while I was doing that, I realized that this patch is wrong. The patch should be like this: writel(0x0, >pl310_tag_latency_ctrl); writel(0x010, >pl310_data_latency_ctrl); The reason is for the latency values: 000 = 1 cycle of latency, there is no additional latency. 001 = 2 cycles of latency. 010 = 3 cycles of latency. 011 = 4 cycles of latency. 100 = 5 cycles of latency. 101 = 6 cycles of latency. 110 = 7 cycles of latency. 111 = 8 cycles of latency. So from the values in the device tree, they should be n-1. It looks like you've already sent the patch to Tom. I'll send a follow up patch to fix that when it lands. >>> >>> Drat, thanks. >>> >>> Better yet, pull the latency config into a function, so it can be used >>> by other platforms. The prototype should take 7 parameters, address and >>> latency in cycles, so that it shields the users from this n-1 stuff. >>> >> >> Agreed. I'm working on an RFC patch that creates a UBOOT_MISC driver to >> handle all of the pl310 settings. Hope to send it out sometime next week. > > I'd like a simpler fix for this release if possible, and a subsequent > patch for the DM conversion. > ok.. Dinh ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] ARM: socfpga: Configure PL310 latencies
On 3/1/19 4:19 PM, Dinh Nguyen wrote: > > > On 3/1/19 3:40 AM, Marek Vasut wrote: >> On 3/1/19 12:59 AM, Dinh Nguyen wrote: >>> Hi Marek, >>> >>> On 2/19/19 4:01 AM, Simon Goldschmidt wrote: On Tue, Feb 19, 2019 at 1:44 AM Marek Vasut wrote: > > Configure the PL310 tag and data latency registers, which slightly > improves performance and aligns the behavior with Linux. > > Signed-off-by: Marek Vasut > Cc: Dalon Westergreen > Cc: Dinh Nguyen > --- > arch/arm/mach-socfpga/misc.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c > index 78fbe28724..1ea4e32c11 100644 > --- a/arch/arm/mach-socfpga/misc.c > +++ b/arch/arm/mach-socfpga/misc.c > @@ -62,6 +62,9 @@ void v7_outer_cache_enable(void) > /* Disable the L2 cache */ > clrbits_le32(>pl310_ctrl, L2X0_CTRL_EN); > > + writel(0x111, >pl310_tag_latency_ctrl); > + writel(0x121, >pl310_data_latency_ctrl); Would it make sense to add defines as named constants for this? OTOH, in Linux, the values in the devicetree aren't really described, either, so: >>> >>> I was thinking the same, so I'm working on a patch to get these values >>> from the device tree. >>> >>> So while I was doing that, I realized that this patch is wrong. >>> >>> The patch should be like this: >>> >>> writel(0x0, >pl310_tag_latency_ctrl); >>> writel(0x010, >pl310_data_latency_ctrl); >>> >>> The reason is for the latency values: >>> >>> 000 = 1 cycle of latency, there is no additional latency. >>> 001 = 2 cycles of latency. >>> 010 = 3 cycles of latency. >>> 011 = 4 cycles of latency. >>> 100 = 5 cycles of latency. >>> 101 = 6 cycles of latency. >>> 110 = 7 cycles of latency. >>> 111 = 8 cycles of latency. >>> >>> So from the values in the device tree, they should be n-1. >>> >>> It looks like you've already sent the patch to Tom. I'll send a follow >>> up patch to fix that when it lands. >> >> Drat, thanks. >> >> Better yet, pull the latency config into a function, so it can be used >> by other platforms. The prototype should take 7 parameters, address and >> latency in cycles, so that it shields the users from this n-1 stuff. >> > > Agreed. I'm working on an RFC patch that creates a UBOOT_MISC driver to > handle all of the pl310 settings. Hope to send it out sometime next week. I'd like a simpler fix for this release if possible, and a subsequent patch for the DM conversion. -- Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] ARM: socfpga: Configure PL310 latencies
On 3/1/19 3:40 AM, Marek Vasut wrote: > On 3/1/19 12:59 AM, Dinh Nguyen wrote: >> Hi Marek, >> >> On 2/19/19 4:01 AM, Simon Goldschmidt wrote: >>> On Tue, Feb 19, 2019 at 1:44 AM Marek Vasut wrote: Configure the PL310 tag and data latency registers, which slightly improves performance and aligns the behavior with Linux. Signed-off-by: Marek Vasut Cc: Dalon Westergreen Cc: Dinh Nguyen --- arch/arm/mach-socfpga/misc.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c index 78fbe28724..1ea4e32c11 100644 --- a/arch/arm/mach-socfpga/misc.c +++ b/arch/arm/mach-socfpga/misc.c @@ -62,6 +62,9 @@ void v7_outer_cache_enable(void) /* Disable the L2 cache */ clrbits_le32(>pl310_ctrl, L2X0_CTRL_EN); + writel(0x111, >pl310_tag_latency_ctrl); + writel(0x121, >pl310_data_latency_ctrl); >>> >>> Would it make sense to add defines as named constants for this? >>> OTOH, in Linux, the values in the devicetree aren't really described, >>> either, so: >> >> I was thinking the same, so I'm working on a patch to get these values >> from the device tree. >> >> So while I was doing that, I realized that this patch is wrong. >> >> The patch should be like this: >> >> writel(0x0, >pl310_tag_latency_ctrl); >> writel(0x010, >pl310_data_latency_ctrl); >> >> The reason is for the latency values: >> >> 000 = 1 cycle of latency, there is no additional latency. >> 001 = 2 cycles of latency. >> 010 = 3 cycles of latency. >> 011 = 4 cycles of latency. >> 100 = 5 cycles of latency. >> 101 = 6 cycles of latency. >> 110 = 7 cycles of latency. >> 111 = 8 cycles of latency. >> >> So from the values in the device tree, they should be n-1. >> >> It looks like you've already sent the patch to Tom. I'll send a follow >> up patch to fix that when it lands. > > Drat, thanks. > > Better yet, pull the latency config into a function, so it can be used > by other platforms. The prototype should take 7 parameters, address and > latency in cycles, so that it shields the users from this n-1 stuff. > Agreed. I'm working on an RFC patch that creates a UBOOT_MISC driver to handle all of the pl310 settings. Hope to send it out sometime next week. Dinh ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] ARM: socfpga: Configure PL310 latencies
On 3/1/19 12:59 AM, Dinh Nguyen wrote: > Hi Marek, > > On 2/19/19 4:01 AM, Simon Goldschmidt wrote: >> On Tue, Feb 19, 2019 at 1:44 AM Marek Vasut wrote: >>> >>> Configure the PL310 tag and data latency registers, which slightly >>> improves performance and aligns the behavior with Linux. >>> >>> Signed-off-by: Marek Vasut >>> Cc: Dalon Westergreen >>> Cc: Dinh Nguyen >>> --- >>> arch/arm/mach-socfpga/misc.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c >>> index 78fbe28724..1ea4e32c11 100644 >>> --- a/arch/arm/mach-socfpga/misc.c >>> +++ b/arch/arm/mach-socfpga/misc.c >>> @@ -62,6 +62,9 @@ void v7_outer_cache_enable(void) >>> /* Disable the L2 cache */ >>> clrbits_le32(>pl310_ctrl, L2X0_CTRL_EN); >>> >>> + writel(0x111, >pl310_tag_latency_ctrl); >>> + writel(0x121, >pl310_data_latency_ctrl); >> >> Would it make sense to add defines as named constants for this? >> OTOH, in Linux, the values in the devicetree aren't really described, >> either, so: > > I was thinking the same, so I'm working on a patch to get these values > from the device tree. > > So while I was doing that, I realized that this patch is wrong. > > The patch should be like this: > > writel(0x0, >pl310_tag_latency_ctrl); > writel(0x010, >pl310_data_latency_ctrl); > > The reason is for the latency values: > > 000 = 1 cycle of latency, there is no additional latency. > 001 = 2 cycles of latency. > 010 = 3 cycles of latency. > 011 = 4 cycles of latency. > 100 = 5 cycles of latency. > 101 = 6 cycles of latency. > 110 = 7 cycles of latency. > 111 = 8 cycles of latency. > > So from the values in the device tree, they should be n-1. > > It looks like you've already sent the patch to Tom. I'll send a follow > up patch to fix that when it lands. Drat, thanks. Better yet, pull the latency config into a function, so it can be used by other platforms. The prototype should take 7 parameters, address and latency in cycles, so that it shields the users from this n-1 stuff. -- Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] ARM: socfpga: Configure PL310 latencies
Hi Marek, On 2/19/19 4:01 AM, Simon Goldschmidt wrote: > On Tue, Feb 19, 2019 at 1:44 AM Marek Vasut wrote: >> >> Configure the PL310 tag and data latency registers, which slightly >> improves performance and aligns the behavior with Linux. >> >> Signed-off-by: Marek Vasut >> Cc: Dalon Westergreen >> Cc: Dinh Nguyen >> --- >> arch/arm/mach-socfpga/misc.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c >> index 78fbe28724..1ea4e32c11 100644 >> --- a/arch/arm/mach-socfpga/misc.c >> +++ b/arch/arm/mach-socfpga/misc.c >> @@ -62,6 +62,9 @@ void v7_outer_cache_enable(void) >> /* Disable the L2 cache */ >> clrbits_le32(>pl310_ctrl, L2X0_CTRL_EN); >> >> + writel(0x111, >pl310_tag_latency_ctrl); >> + writel(0x121, >pl310_data_latency_ctrl); > > Would it make sense to add defines as named constants for this? > OTOH, in Linux, the values in the devicetree aren't really described, > either, so: I was thinking the same, so I'm working on a patch to get these values from the device tree. So while I was doing that, I realized that this patch is wrong. The patch should be like this: writel(0x0, >pl310_tag_latency_ctrl); writel(0x010, >pl310_data_latency_ctrl); The reason is for the latency values: 000 = 1 cycle of latency, there is no additional latency. 001 = 2 cycles of latency. 010 = 3 cycles of latency. 011 = 4 cycles of latency. 100 = 5 cycles of latency. 101 = 6 cycles of latency. 110 = 7 cycles of latency. 111 = 8 cycles of latency. So from the values in the device tree, they should be n-1. It looks like you've already sent the patch to Tom. I'll send a follow up patch to fix that when it lands. Dinh ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] ARM: socfpga: Configure PL310 latencies
On 2/18/19 6:44 PM, Marek Vasut wrote: > Configure the PL310 tag and data latency registers, which slightly > improves performance and aligns the behavior with Linux. > > Signed-off-by: Marek Vasut > Cc: Dalon Westergreen > Cc: Dinh Nguyen > --- > arch/arm/mach-socfpga/misc.c | 3 +++ > 1 file changed, 3 insertions(+) > Looks good! Reviewed-by: Dinh Nguyen ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] ARM: socfpga: Configure PL310 latencies
On Tue, Feb 19, 2019 at 1:44 AM Marek Vasut wrote: > > Configure the PL310 tag and data latency registers, which slightly > improves performance and aligns the behavior with Linux. > > Signed-off-by: Marek Vasut > Cc: Dalon Westergreen > Cc: Dinh Nguyen > --- > arch/arm/mach-socfpga/misc.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c > index 78fbe28724..1ea4e32c11 100644 > --- a/arch/arm/mach-socfpga/misc.c > +++ b/arch/arm/mach-socfpga/misc.c > @@ -62,6 +62,9 @@ void v7_outer_cache_enable(void) > /* Disable the L2 cache */ > clrbits_le32(>pl310_ctrl, L2X0_CTRL_EN); > > + writel(0x111, >pl310_tag_latency_ctrl); > + writel(0x121, >pl310_data_latency_ctrl); Would it make sense to add defines as named constants for this? OTOH, in Linux, the values in the devicetree aren't really described, either, so: Reviewed-by: Simon Goldschmidt Regards, Simon > + > /* enable BRESP, instruction and data prefetch, full line of zeroes */ > setbits_le32(>pl310_aux_ctrl, > L310_AUX_CTRL_DATA_PREFETCH_MASK | > -- > 2.19.2 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH] ARM: socfpga: Configure PL310 latencies
Configure the PL310 tag and data latency registers, which slightly improves performance and aligns the behavior with Linux. Signed-off-by: Marek Vasut Cc: Dalon Westergreen Cc: Dinh Nguyen --- arch/arm/mach-socfpga/misc.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c index 78fbe28724..1ea4e32c11 100644 --- a/arch/arm/mach-socfpga/misc.c +++ b/arch/arm/mach-socfpga/misc.c @@ -62,6 +62,9 @@ void v7_outer_cache_enable(void) /* Disable the L2 cache */ clrbits_le32(>pl310_ctrl, L2X0_CTRL_EN); + writel(0x111, >pl310_tag_latency_ctrl); + writel(0x121, >pl310_data_latency_ctrl); + /* enable BRESP, instruction and data prefetch, full line of zeroes */ setbits_le32(>pl310_aux_ctrl, L310_AUX_CTRL_DATA_PREFETCH_MASK | -- 2.19.2 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
