H Cedric,
> Subject: Re: [PATCH v1 2/3] hw/timer/aspeed: Add AST2700 Support
>
> On 12/16/24 08:53, Jamin Lin wrote:
> > The timer controller include 8 sets of 32-bit decrement counters,
> > based on either PCLK or 1MHZ clock and the design of timer controller
> > between AST2600 and AST2700 are almost the same.
> >
> > The different is that the register set have a significant change in AST2700.
>
> You can drop the sentence above ^. It seems redundant.
>
> > TIMER0 – TIMER7 has their own individua control and interrupt status
> register.
>
> individual
>
> > In other words, users are able to set timer control in register TMC10
> > with different TIMER base address and clear timer control and
> > interrupt status in register TMC14 with different TIMER base address.
> >
> > Both "aspeed_timer_read" and "aspeed_timer_write" callback functions
> > are not compatible AST2700. Introduce new "aspeed_2700_timer_read" and
> > "aspeed_2700_timer_write" callback functions and
> > "aspeed_2700_timer_ops" memory region operation for AST2700. Introduce
> a new ast2700 class to support AST2700.
> >
> > The base address of TIMER0 to TIMER7 as following.
> > Base Address of Timer 0 = 0x12C1_0000
> > Base Address of Timer 1 = 0x12C1_0040
> > Base Address of Timer 2 = 0x12C1_0080
> > Base Address of Timer 3 = 0x12C1_00C0
> > Base Address of Timer 4 = 0x12C1_0100
> > Base Address of Timer 5 = 0x12C1_0140
> > Base Address of Timer 6 = 0x12C1_0180
> > Base Address of Timer 7 = 0x12C1_01C0
> >
> > The register address space of each TIMER is "0x40" , so uses the
> > following
>
> I think this change would improve reading :
>
> " , so uses" -> "and uses"
>
> > formula to get the index and register of each TIMER.
> >
> > timer_index = offset >> 6;
> > timer_offset = offset & 0x3f;
> >
> > The TMC010 is a counter control set and interrupt status register.
> > Write "1" to TMC10[3:0] will set the specific bits to "1". Introduce a
> > new "aspeed_2700_timer_set_ctrl" function to handle this register behavior.
> >
> > The TMC014 is a counter control clear and interrupt status register,
> > to clear the specific bits to "0", it should write "1" to TMC14[3:0]
> > on the same bit position. Introduce a new
> > "aspeed_2700_timer_clear_ctrl" function to handle this register behavior.
> TMC014 does not support read operation.
> >
> > Signed-off-by: Jamin Lin <jamin_...@aspeedtech.com>
>
> The rest looks OK. So,
>
> Acked-by: Cédric Le Goater <c...@redhat.com>
>
> I would prefer Andrew to take look before merging this change though.
>
Got it.
Thanks for your review, suggestion and kindly support.
Jamin
> Thanks,
>
> C.
>
>
> > ---
> > hw/timer/aspeed_timer.c | 224
> ++++++++++++++++++++++++++++++++
> > include/hw/timer/aspeed_timer.h | 1 +
> > 2 files changed, 225 insertions(+)
> >
> > diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c index
> > 970bf1d79d..de4d78a0fb 100644
> > --- a/hw/timer/aspeed_timer.c
> > +++ b/hw/timer/aspeed_timer.c
> > @@ -593,6 +593,214 @@ static void
> aspeed_2600_timer_write(AspeedTimerCtrlState *s, hwaddr offset,
> > }
> > }
> >
> > +static void aspeed_2700_timer_set_ctrl(AspeedTimerCtrlState *s, int index,
> > + uint32_t reg) {
> > + const uint8_t overflow_interrupt_mask = BIT(op_overflow_interrupt);
> > + const uint8_t external_clock_mask = BIT(op_external_clock);
> > + const uint8_t pulse_enable_mask = BIT(op_pulse_enable);
> > + const uint8_t enable_mask = BIT(op_enable);
> > + AspeedTimer *t;
> > + uint8_t t_old;
> > + uint8_t t_new;
> > + int shift;
> > +
> > + /*
> > + * Only 1 will set the specific bits to 1
> > + * Handle a dependency between the 'enable' and remaining three
> > + * configuration bits - i.e. if more than one bit in the control set
> > has
> > + * set, including the 'enable' bit, perform configuration and then
> > + * enable the timer.
> > + * Interrupt Status bit should not be set.
> > + */
> > +
> > + t = &s->timers[index];
> > + shift = index * TIMER_CTRL_BITS;
> > +
> > + t_old = (s->ctrl >> shift) & TIMER_CTRL_MASK;
> > + t_new = reg & TIMER_CTRL_MASK;
> > +
> > + if (!(t_old & external_clock_mask) &&
> > + (t_new & external_clock_mask)) {
> > + aspeed_timer_ctrl_external_clock(t, true);
> > + s->ctrl = deposit32(s->ctrl, shift + op_external_clock, 1, 1);
> > + }
> > +
> > + if (!(t_old & overflow_interrupt_mask) &&
> > + (t_new & overflow_interrupt_mask)) {
> > + aspeed_timer_ctrl_overflow_interrupt(t, true);
> > + s->ctrl = deposit32(s->ctrl, shift + op_overflow_interrupt, 1, 1);
> > + }
> > +
> > +
> > + if (!(t_old & pulse_enable_mask) &&
> > + (t_new & pulse_enable_mask)) {
> > + aspeed_timer_ctrl_pulse_enable(t, true);
> > + s->ctrl = deposit32(s->ctrl, shift + op_pulse_enable, 1, 1);
> > + }
> > +
> > + /* If we are enabling, do so last */
> > + if (!(t_old & enable_mask) &&
> > + (t_new & enable_mask)) {
> > + aspeed_timer_ctrl_enable(t, true);
> > + s->ctrl = deposit32(s->ctrl, shift + op_enable, 1, 1);
> > + }
> > +}
> > +
> > +static void aspeed_2700_timer_clear_ctrl(AspeedTimerCtrlState *s, int
> index,
> > + uint32_t reg) {
> > + const uint8_t overflow_interrupt_mask = BIT(op_overflow_interrupt);
> > + const uint8_t external_clock_mask = BIT(op_external_clock);
> > + const uint8_t pulse_enable_mask = BIT(op_pulse_enable);
> > + const uint8_t enable_mask = BIT(op_enable);
> > + AspeedTimer *t;
> > + uint8_t t_old;
> > + uint8_t t_new;
> > + int shift;
> > +
> > + /*
> > + * Only 1 will clear the specific bits to 0
> > + * Handle a dependency between the 'enable' and remaining three
> > + * configuration bits - i.e. if more than one bit in the control set
> > has
> > + * clear, including the 'enable' bit, then disable the timer and
> > perform
> > + * configuration
> > + */
> > +
> > + t = &s->timers[index];
> > + shift = index * TIMER_CTRL_BITS;
> > +
> > + t_old = (s->ctrl >> shift) & TIMER_CTRL_MASK;
> > + t_new = reg & TIMER_CTRL_MASK;
> > +
> > + /* If we are disabling, do so first */
> > + if ((t_old & enable_mask) &&
> > + (t_new & enable_mask)) {
> > + aspeed_timer_ctrl_enable(t, false);
> > + s->ctrl = deposit32(s->ctrl, shift + op_enable, 1, 0);
> > + }
> > +
> > + if ((t_old & external_clock_mask) &&
> > + (t_new & external_clock_mask)) {
> > + aspeed_timer_ctrl_external_clock(t, false);
> > + s->ctrl = deposit32(s->ctrl, shift + op_external_clock, 1, 0);
> > + }
> > +
> > + if ((t_old & overflow_interrupt_mask) &&
> > + (t_new & overflow_interrupt_mask)) {
> > + aspeed_timer_ctrl_overflow_interrupt(t, false);
> > + s->ctrl = deposit32(s->ctrl, shift + op_overflow_interrupt, 1, 0);
> > + }
> > +
> > + if ((t_old & pulse_enable_mask) &&
> > + (t_new & pulse_enable_mask)) {
> > + aspeed_timer_ctrl_pulse_enable(t, false);
> > + s->ctrl = deposit32(s->ctrl, shift + op_pulse_enable, 1, 0);
> > + }
> > +
> > + if ((t_old & pulse_enable_mask) &&
> > + (t_new & pulse_enable_mask)) {
> > + aspeed_timer_ctrl_pulse_enable(t, false);
> > + s->ctrl = deposit32(s->ctrl, shift + op_pulse_enable, 1, 0);
> > + }
> > +
> > + /* Clear interrupt status */
> > + if (reg & 0x10000) {
> > + s->irq_sts = deposit32(s->irq_sts, index, 1, 0);
> > + }
> > +}
> > +
> > +static uint64_t aspeed_2700_timer_read(void *opaque, hwaddr offset,
> > + uint32_t size) {
> > + AspeedTimerCtrlState *s = ASPEED_TIMER(opaque);
> > + uint32_t timer_offset = offset & 0x3f;
> > + int timer_index = offset >> 6;
> > + uint64_t value = 0;
> > +
> > + if (timer_index >= ASPEED_TIMER_NR_TIMERS) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: offset 0x%" PRIx64 " out of bounds\n",
> > + __func__, offset);
> > + return 0;
> > + }
> > +
> > + switch (timer_offset) {
> > + /*
> > + * Counter Status
> > + * Counter Reload
> > + * Counter First Matching
> > + * Counter Second Matching
> > + */
> > + case 0x00 ... 0x0C:
> > + value = aspeed_timer_get_value(&s->timers[timer_index],
> > + timer_offset >> 2);
> > + break;
> > + /* Counter Control and Interrupt Status */
> > + case 0x10:
> > + value = deposit64(value, 0, 4,
> > + extract32(s->ctrl, timer_index * 4, 4));
> > + value = deposit64(value, 16, 1,
> > + extract32(s->irq_sts, timer_index, 1));
> > + break;
> > + default:
> > + qemu_log_mask(LOG_GUEST_ERROR, "%s: no getter for offset
> 0x%"
> > + PRIx64"\n", __func__, offset);
> > + return 0;
> > + }
> > + trace_aspeed_timer_read(offset, size, value);
> > + return value;
> > +}
> > +
> > +static void aspeed_2700_timer_write(void *opaque, hwaddr offset,
> > + uint64_t value, uint32_t size) {
> > + const uint32_t timer_value = (uint32_t)(value & 0xFFFFFFFF);
> > + AspeedTimerCtrlState *s = ASPEED_TIMER(opaque);
> > + uint32_t timer_offset = offset & 0x3f;
> > + int timer_index = offset >> 6;
> > +
> > + if (timer_index >= ASPEED_TIMER_NR_TIMERS) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: offset 0x%" PRIx64 " out of bounds\n",
> > + __func__, offset);
> > + }
> > +
> > + switch (timer_offset) {
> > + /*
> > + * Counter Status
> > + * Counter Reload
> > + * Counter First Matching
> > + * Counter Second Matching
> > + */
> > + case 0x00 ... 0x0C:
> > + aspeed_timer_set_value(s, timer_index, timer_offset >> 2,
> > + timer_value);
> > + break;
> > + /* Counter Control Set and Interrupt Status */
> > + case 0x10:
> > + aspeed_2700_timer_set_ctrl(s, timer_index, timer_value);
> > + break;
> > + /* Counter Control Clear and Interrupr Status */
> > + case 0x14:
> > + aspeed_2700_timer_clear_ctrl(s, timer_index, timer_value);
> > + break;
> > + default:
> > + qemu_log_mask(LOG_GUEST_ERROR, "%s: no setter for offset
> 0x%"
> > + PRIx64"\n", __func__, offset);
> > + break;
> > + }
> > +}
> > +
> > +static const MemoryRegionOps aspeed_2700_timer_ops = {
> > + .read = aspeed_2700_timer_read,
> > + .write = aspeed_2700_timer_write,
> > + .endianness = DEVICE_LITTLE_ENDIAN,
> > + .valid.min_access_size = 4,
> > + .valid.max_access_size = 4,
> > + .valid.unaligned = false,
> > +};
> > +
> > static void aspeed_init_one_timer(AspeedTimerCtrlState *s, uint8_t id)
> > {
> > AspeedTimer *t = &s->timers[id]; @@ -769,6 +977,21 @@ static
> > const TypeInfo aspeed_1030_timer_info = {
> > .class_init = aspeed_1030_timer_class_init,
> > };
> >
> > +static void aspeed_2700_timer_class_init(ObjectClass *klass, void
> > +*data) {
> > + DeviceClass *dc = DEVICE_CLASS(klass);
> > + AspeedTimerClass *awc = ASPEED_TIMER_CLASS(klass);
> > +
> > + dc->desc = "ASPEED 2700 Timer";
> > + awc->reg_ops = &aspeed_2700_timer_ops; }
> > +
> > +static const TypeInfo aspeed_2700_timer_info = {
> > + .name = TYPE_ASPEED_2700_TIMER,
> > + .parent = TYPE_ASPEED_TIMER,
> > + .class_init = aspeed_2700_timer_class_init, };
> > +
> > static void aspeed_timer_register_types(void)
> > {
> > type_register_static(&aspeed_timer_info);
> > @@ -776,6 +999,7 @@ static void aspeed_timer_register_types(void)
> > type_register_static(&aspeed_2500_timer_info);
> > type_register_static(&aspeed_2600_timer_info);
> > type_register_static(&aspeed_1030_timer_info);
> > + type_register_static(&aspeed_2700_timer_info);
> > }
> >
> > type_init(aspeed_timer_register_types)
> > diff --git a/include/hw/timer/aspeed_timer.h
> > b/include/hw/timer/aspeed_timer.h index 8d0b15f055..2247dc20ab 100644
> > --- a/include/hw/timer/aspeed_timer.h
> > +++ b/include/hw/timer/aspeed_timer.h
> > @@ -32,6 +32,7 @@ OBJECT_DECLARE_TYPE(AspeedTimerCtrlState,
> AspeedTimerClass, ASPEED_TIMER)
> > #define TYPE_ASPEED_2500_TIMER TYPE_ASPEED_TIMER "-ast2500"
> > #define TYPE_ASPEED_2600_TIMER TYPE_ASPEED_TIMER "-ast2600"
> > #define TYPE_ASPEED_1030_TIMER TYPE_ASPEED_TIMER "-ast1030"
> > +#define TYPE_ASPEED_2700_TIMER TYPE_ASPEED_TIMER "-ast2700"
> >
> > #define ASPEED_TIMER_NR_TIMERS 8
> >
