| Message ID | 20221230113504.37032-5-philmd@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/arm/aspeed_ast10x0: Map more peripherals & few more fixes | expand |
On 12/30/22 12:34, Philippe Mathieu-Daudé wrote: > IEC binary prefixes ease code review: the unit is explicit. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Reviewed-by: Peter Delevoryas <peter@pjd.dev> Reviewed-by: Cédric Le Goater <clg@kaod.org> Thanks, C. > --- > hw/arm/aspeed_ast10x0.c | 3 ++- > hw/arm/aspeed_ast2600.c | 3 ++- > hw/arm/aspeed_soc.c | 4 ++-- > 3 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c > index 122b3fd3f3..3500294df7 100644 > --- a/hw/arm/aspeed_ast10x0.c > +++ b/hw/arm/aspeed_ast10x0.c > @@ -10,6 +10,7 @@ > */ > > #include "qemu/osdep.h" > +#include "qemu/units.h" > #include "qapi/error.h" > #include "exec/address-spaces.h" > #include "sysemu/sysemu.h" > @@ -348,7 +349,7 @@ static void aspeed_soc_ast1030_class_init(ObjectClass *klass, void *data) > sc->name = "ast1030-a1"; > sc->cpu_type = ARM_CPU_TYPE_NAME("cortex-m4"); > sc->silicon_rev = AST1030_A1_SILICON_REV; > - sc->sram_size = 0xc0000; > + sc->sram_size = 768 * KiB; > sc->spis_num = 2; > sc->ehcis_num = 0; > sc->wdts_num = 4; > diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c > index a79e05ddbd..72df72a540 100644 > --- a/hw/arm/aspeed_ast2600.c > +++ b/hw/arm/aspeed_ast2600.c > @@ -8,6 +8,7 @@ > */ > > #include "qemu/osdep.h" > +#include "qemu/units.h" > #include "qapi/error.h" > #include "hw/misc/unimp.h" > #include "hw/arm/aspeed_soc.h" > @@ -619,7 +620,7 @@ static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data) > sc->name = "ast2600-a3"; > sc->cpu_type = ARM_CPU_TYPE_NAME("cortex-a7"); > sc->silicon_rev = AST2600_A3_SILICON_REV; > - sc->sram_size = 0x16400; > + sc->sram_size = 89 * KiB; > sc->spis_num = 2; > sc->ehcis_num = 2; > sc->wdts_num = 4; > diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c > index 2c0924d311..677342c9ed 100644 > --- a/hw/arm/aspeed_soc.c > +++ b/hw/arm/aspeed_soc.c > @@ -517,7 +517,7 @@ static void aspeed_soc_ast2400_class_init(ObjectClass *oc, void *data) > sc->name = "ast2400-a1"; > sc->cpu_type = ARM_CPU_TYPE_NAME("arm926"); > sc->silicon_rev = AST2400_A1_SILICON_REV; > - sc->sram_size = 0x8000; > + sc->sram_size = 32 * KiB; > sc->spis_num = 1; > sc->ehcis_num = 1; > sc->wdts_num = 2; > @@ -544,7 +544,7 @@ static void aspeed_soc_ast2500_class_init(ObjectClass *oc, void *data) > sc->name = "ast2500-a1"; > sc->cpu_type = ARM_CPU_TYPE_NAME("arm1176"); > sc->silicon_rev = AST2500_A1_SILICON_REV; > - sc->sram_size = 0x9000; > + sc->sram_size = 36 * KiB; > sc->spis_num = 2; > sc->ehcis_num = 2; > sc->wdts_num = 3;
On Fri, 30 Dec 2022 at 11:35, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > IEC binary prefixes ease code review: the unit is explicit. I strongly prefer the existing code; it tells you the size without having to do maths. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Reviewed-by: Peter Delevoryas <peter@pjd.dev> > --- > hw/arm/aspeed_ast10x0.c | 3 ++- > hw/arm/aspeed_ast2600.c | 3 ++- > hw/arm/aspeed_soc.c | 4 ++-- > 3 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c > index 122b3fd3f3..3500294df7 100644 > --- a/hw/arm/aspeed_ast10x0.c > +++ b/hw/arm/aspeed_ast10x0.c > @@ -10,6 +10,7 @@ > */ > > #include "qemu/osdep.h" > +#include "qemu/units.h" > #include "qapi/error.h" > #include "exec/address-spaces.h" > #include "sysemu/sysemu.h" > @@ -348,7 +349,7 @@ static void aspeed_soc_ast1030_class_init(ObjectClass *klass, void *data) > sc->name = "ast1030-a1"; > sc->cpu_type = ARM_CPU_TYPE_NAME("cortex-m4"); > sc->silicon_rev = AST1030_A1_SILICON_REV; > - sc->sram_size = 0xc0000; > + sc->sram_size = 768 * KiB; > sc->spis_num = 2; > sc->ehcis_num = 0; > sc->wdts_num = 4; > diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c > index a79e05ddbd..72df72a540 100644 > --- a/hw/arm/aspeed_ast2600.c > +++ b/hw/arm/aspeed_ast2600.c > @@ -8,6 +8,7 @@ > */ > > #include "qemu/osdep.h" > +#include "qemu/units.h" > #include "qapi/error.h" > #include "hw/misc/unimp.h" > #include "hw/arm/aspeed_soc.h" > @@ -619,7 +620,7 @@ static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data) > sc->name = "ast2600-a3"; > sc->cpu_type = ARM_CPU_TYPE_NAME("cortex-a7"); > sc->silicon_rev = AST2600_A3_SILICON_REV; > - sc->sram_size = 0x16400; > + sc->sram_size = 89 * KiB; > sc->spis_num = 2; > sc->ehcis_num = 2; > sc->wdts_num = 4; > diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c > index 2c0924d311..677342c9ed 100644 > --- a/hw/arm/aspeed_soc.c > +++ b/hw/arm/aspeed_soc.c > @@ -517,7 +517,7 @@ static void aspeed_soc_ast2400_class_init(ObjectClass *oc, void *data) > sc->name = "ast2400-a1"; > sc->cpu_type = ARM_CPU_TYPE_NAME("arm926"); > sc->silicon_rev = AST2400_A1_SILICON_REV; > - sc->sram_size = 0x8000; > + sc->sram_size = 32 * KiB; > sc->spis_num = 1; > sc->ehcis_num = 1; > sc->wdts_num = 2; > @@ -544,7 +544,7 @@ static void aspeed_soc_ast2500_class_init(ObjectClass *oc, void *data) > sc->name = "ast2500-a1"; > sc->cpu_type = ARM_CPU_TYPE_NAME("arm1176"); > sc->silicon_rev = AST2500_A1_SILICON_REV; > - sc->sram_size = 0x9000; > + sc->sram_size = 36 * KiB; > sc->spis_num = 2; > sc->ehcis_num = 2; > sc->wdts_num = 3; > -- > 2.38.1 >
On 1/18/23 07:53, Joel Stanley wrote: > On Fri, 30 Dec 2022 at 11:35, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> >> IEC binary prefixes ease code review: the unit is explicit. > > I strongly prefer the existing code; it tells you the size without > having to do maths. you mean that it matches better with the address space representation in the code and the 'info mtree' output ? If so, I agree. We can keep this patch out, it is not fundamental. The hex representation of values has its advantages compared to the macros because hex is generally what you get in debug outputs and it is easier to compare and manipulate. Some Linux dev feel the same. C. > >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> Reviewed-by: Peter Delevoryas <peter@pjd.dev> >> --- >> hw/arm/aspeed_ast10x0.c | 3 ++- >> hw/arm/aspeed_ast2600.c | 3 ++- >> hw/arm/aspeed_soc.c | 4 ++-- >> 3 files changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c >> index 122b3fd3f3..3500294df7 100644 >> --- a/hw/arm/aspeed_ast10x0.c >> +++ b/hw/arm/aspeed_ast10x0.c >> @@ -10,6 +10,7 @@ >> */ >> >> #include "qemu/osdep.h" >> +#include "qemu/units.h" >> #include "qapi/error.h" >> #include "exec/address-spaces.h" >> #include "sysemu/sysemu.h" >> @@ -348,7 +349,7 @@ static void aspeed_soc_ast1030_class_init(ObjectClass *klass, void *data) >> sc->name = "ast1030-a1"; >> sc->cpu_type = ARM_CPU_TYPE_NAME("cortex-m4"); >> sc->silicon_rev = AST1030_A1_SILICON_REV; >> - sc->sram_size = 0xc0000; >> + sc->sram_size = 768 * KiB; >> sc->spis_num = 2; >> sc->ehcis_num = 0; >> sc->wdts_num = 4; >> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c >> index a79e05ddbd..72df72a540 100644 >> --- a/hw/arm/aspeed_ast2600.c >> +++ b/hw/arm/aspeed_ast2600.c >> @@ -8,6 +8,7 @@ >> */ >> >> #include "qemu/osdep.h" >> +#include "qemu/units.h" >> #include "qapi/error.h" >> #include "hw/misc/unimp.h" >> #include "hw/arm/aspeed_soc.h" >> @@ -619,7 +620,7 @@ static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data) >> sc->name = "ast2600-a3"; >> sc->cpu_type = ARM_CPU_TYPE_NAME("cortex-a7"); >> sc->silicon_rev = AST2600_A3_SILICON_REV; >> - sc->sram_size = 0x16400; >> + sc->sram_size = 89 * KiB; >> sc->spis_num = 2; >> sc->ehcis_num = 2; >> sc->wdts_num = 4; >> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c >> index 2c0924d311..677342c9ed 100644 >> --- a/hw/arm/aspeed_soc.c >> +++ b/hw/arm/aspeed_soc.c >> @@ -517,7 +517,7 @@ static void aspeed_soc_ast2400_class_init(ObjectClass *oc, void *data) >> sc->name = "ast2400-a1"; >> sc->cpu_type = ARM_CPU_TYPE_NAME("arm926"); >> sc->silicon_rev = AST2400_A1_SILICON_REV; >> - sc->sram_size = 0x8000; >> + sc->sram_size = 32 * KiB; >> sc->spis_num = 1; >> sc->ehcis_num = 1; >> sc->wdts_num = 2; >> @@ -544,7 +544,7 @@ static void aspeed_soc_ast2500_class_init(ObjectClass *oc, void *data) >> sc->name = "ast2500-a1"; >> sc->cpu_type = ARM_CPU_TYPE_NAME("arm1176"); >> sc->silicon_rev = AST2500_A1_SILICON_REV; >> - sc->sram_size = 0x9000; >> + sc->sram_size = 36 * KiB; >> sc->spis_num = 2; >> sc->ehcis_num = 2; >> sc->wdts_num = 3; >> -- >> 2.38.1 >>
On 18/1/23 08:32, Cédric Le Goater wrote: > On 1/18/23 07:53, Joel Stanley wrote: >> On Fri, 30 Dec 2022 at 11:35, Philippe Mathieu-Daudé >> <philmd@linaro.org> wrote: >>> >>> IEC binary prefixes ease code review: the unit is explicit. >> >> I strongly prefer the existing code; it tells you the size without >> having to do maths. I guess it depends on one's mindset and culture / education, maybe I'm too young for the full hexadecimal world and am more custom to decimal notation with binary prefixes. 0x16400 is just another magic number for me, while 89 * KiB is a size. > you mean that it matches better with the address space representation > in the code and the 'info mtree' output ? If so, I agree. We can keep > this patch out, it is not fundamental. > > The hex representation of values has its advantages compared to the > macros because hex is generally what you get in debug outputs and > it is easier to compare and manipulate. Some Linux dev feel the > same. >>> aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data) >>> sc->name = "ast2600-a3"; >>> sc->cpu_type = ARM_CPU_TYPE_NAME("cortex-a7"); >>> sc->silicon_rev = AST2600_A3_SILICON_REV; >>> - sc->sram_size = 0x16400; >>> + sc->sram_size = 89 * KiB; To keep everybody happy I'll respin using: sc->sram_size = 0x16400; /* 89 * KiB */ Thanks, Phil.
On 1/18/23 09:37, Philippe Mathieu-Daudé wrote: > On 18/1/23 08:32, Cédric Le Goater wrote: >> On 1/18/23 07:53, Joel Stanley wrote: >>> On Fri, 30 Dec 2022 at 11:35, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >>>> >>>> IEC binary prefixes ease code review: the unit is explicit. >>> >>> I strongly prefer the existing code; it tells you the size without >>> having to do maths. > > I guess it depends on one's mindset and culture / education, > maybe I'm too young for the full hexadecimal world and am more > custom to decimal notation with binary prefixes. > > 0x16400 is just another magic number for me, while 89 * KiB is a size. > >> you mean that it matches better with the address space representation >> in the code and the 'info mtree' output ? If so, I agree. We can keep >> this patch out, it is not fundamental. >> >> The hex representation of values has its advantages compared to the >> macros because hex is generally what you get in debug outputs and >> it is easier to compare and manipulate. Some Linux dev feel the >> same. > >>>> aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data) >>>> sc->name = "ast2600-a3"; >>>> sc->cpu_type = ARM_CPU_TYPE_NAME("cortex-a7"); >>>> sc->silicon_rev = AST2600_A3_SILICON_REV; >>>> - sc->sram_size = 0x16400; >>>> + sc->sram_size = 89 * KiB; > To keep everybody happy I'll respin using: > > sc->sram_size = 0x16400; /* 89 * KiB */ I had some minor/aesthetic comments. If you could also address them please, I will include this series in the aspeed-8.0 PR. Thanks, C.
diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c index 122b3fd3f3..3500294df7 100644 --- a/hw/arm/aspeed_ast10x0.c +++ b/hw/arm/aspeed_ast10x0.c @@ -10,6 +10,7 @@ */ #include "qemu/osdep.h" +#include "qemu/units.h" #include "qapi/error.h" #include "exec/address-spaces.h" #include "sysemu/sysemu.h" @@ -348,7 +349,7 @@ static void aspeed_soc_ast1030_class_init(ObjectClass *klass, void *data) sc->name = "ast1030-a1"; sc->cpu_type = ARM_CPU_TYPE_NAME("cortex-m4"); sc->silicon_rev = AST1030_A1_SILICON_REV; - sc->sram_size = 0xc0000; + sc->sram_size = 768 * KiB; sc->spis_num = 2; sc->ehcis_num = 0; sc->wdts_num = 4; diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c index a79e05ddbd..72df72a540 100644 --- a/hw/arm/aspeed_ast2600.c +++ b/hw/arm/aspeed_ast2600.c @@ -8,6 +8,7 @@ */ #include "qemu/osdep.h" +#include "qemu/units.h" #include "qapi/error.h" #include "hw/misc/unimp.h" #include "hw/arm/aspeed_soc.h" @@ -619,7 +620,7 @@ static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data) sc->name = "ast2600-a3"; sc->cpu_type = ARM_CPU_TYPE_NAME("cortex-a7"); sc->silicon_rev = AST2600_A3_SILICON_REV; - sc->sram_size = 0x16400; + sc->sram_size = 89 * KiB; sc->spis_num = 2; sc->ehcis_num = 2; sc->wdts_num = 4; diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c index 2c0924d311..677342c9ed 100644 --- a/hw/arm/aspeed_soc.c +++ b/hw/arm/aspeed_soc.c @@ -517,7 +517,7 @@ static void aspeed_soc_ast2400_class_init(ObjectClass *oc, void *data) sc->name = "ast2400-a1"; sc->cpu_type = ARM_CPU_TYPE_NAME("arm926"); sc->silicon_rev = AST2400_A1_SILICON_REV; - sc->sram_size = 0x8000; + sc->sram_size = 32 * KiB; sc->spis_num = 1; sc->ehcis_num = 1; sc->wdts_num = 2; @@ -544,7 +544,7 @@ static void aspeed_soc_ast2500_class_init(ObjectClass *oc, void *data) sc->name = "ast2500-a1"; sc->cpu_type = ARM_CPU_TYPE_NAME("arm1176"); sc->silicon_rev = AST2500_A1_SILICON_REV; - sc->sram_size = 0x9000; + sc->sram_size = 36 * KiB; sc->spis_num = 2; sc->ehcis_num = 2; sc->wdts_num = 3;