Re: [PATCH v3 3/5] Refactor i.MX7 processor code

[Peter Maydell]

On Wed, 2 Aug 2023 at 22:09, Jean-Christophe Dubois <jcd@tribudubois.net> wrote:
>
> * Add Addr and size definition for all i.MX7 devices in i.MX7 header file.
> * Use those newly defined named constants whenever possible.
> * Standardize the way we init a familly of unimplemented devices
>   - SAI
>   - PWM
>   - CAN
> * Add/rework few comments
>
> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
> ---

>      /*
> -     * IOMUXC and IOMUXC_LPSR
> +     * IOMUXC, IOMUXC_GPR and IOMUXC_LPSR
>       */
> -    for (i = 0; i < FSL_IMX7_NUM_IOMUXCS; i++) {
> -        static const hwaddr FSL_IMX7_IOMUXCn_ADDR[FSL_IMX7_NUM_IOMUXCS] = {
> -            FSL_IMX7_IOMUXC_ADDR,
> -            FSL_IMX7_IOMUXC_LPSR_ADDR,
> -        };
> -
> -        snprintf(name, NAME_SIZE, "iomuxc%d", i);
> -        create_unimplemented_device(name, FSL_IMX7_IOMUXCn_ADDR[i],
> -                                    FSL_IMX7_IOMUXCn_SIZE);
> -    }
> +    create_unimplemented_device("iomuxc", FSL_IMX7_IOMUXC_ADDR,
> +                                FSL_IMX7_IOMUXC_SIZE);
> +    create_unimplemented_device("iomuxc_gpr", FSL_IMX7_IOMUXC_GPR_ADDR,
> +                                FSL_IMX7_IOMUXC_GPR_SIZE);
> +    create_unimplemented_device("iomuxc_lspr", FSL_IMX7_IOMUXC_LPSR_ADDR,
> +                                FSL_IMX7_IOMUXC_LPSR_SIZE);

This is a behaviour change -- we used to create 2 stub
iomux devices, and now we create 3. Also, we map the
iomuxc_gpr at FSL_IMX7_IOMUXC_GPR_ADDR here, but we also do

 sysbus_mmio_map(SYS_BUS_DEVICE(&s->gpr), 0, FSL_IMX7_IOMUXC_GPR_ADDR);

below. Which is correct ?


>      create_unimplemented_device("caam", FSL_IMX7_CAAM_ADDR, 
> FSL_IMX7_CAAM_SIZE);
>
>      /*
> -     * PWM
> +     * SAIs (Audio SSI (Synchronous Serial Interface))
>       */
> -    create_unimplemented_device("pwm1", FSL_IMX7_PWM1_ADDR, 
> FSL_IMX7_PWMn_SIZE);
> -    create_unimplemented_device("pwm2", FSL_IMX7_PWM2_ADDR, 
> FSL_IMX7_PWMn_SIZE);
> -    create_unimplemented_device("pwm3", FSL_IMX7_PWM3_ADDR, 
> FSL_IMX7_PWMn_SIZE);
> -    create_unimplemented_device("pwm4", FSL_IMX7_PWM4_ADDR, 
> FSL_IMX7_PWMn_SIZE);
> +    for (i = 0; i < FSL_IMX7_NUM_SAIS; i++) {
> +        static const hwaddr FSL_IMX7_SAIn_ADDR[FSL_IMX7_NUM_SAIS] = {
> +            FSL_IMX7_SAI1_ADDR,
> +            FSL_IMX7_SAI2_ADDR,
> +            FSL_IMX7_SAI3_ADDR,
> +        };
> +
> +        snprintf(name, NAME_SIZE, "sai%d", i);
> +        create_unimplemented_device(name, FSL_IMX7_SAIn_ADDR[i],
> +                                    FSL_IMX7_SAIn_SIZE);
> +    }

Any reason for moving the SAI device creation up like this? It
makes the diff confusing.

>
>      /*
> -     * CAN
> +     * PWMs
>       */
> -    create_unimplemented_device("can1", FSL_IMX7_CAN1_ADDR, 
> FSL_IMX7_CANn_SIZE);
> -    create_unimplemented_device("can2", FSL_IMX7_CAN2_ADDR, 
> FSL_IMX7_CANn_SIZE);
> +    for (i = 0; i < FSL_IMX7_NUM_PWMS; i++) {
> +        static const hwaddr FSL_IMX7_PWMn_ADDR[FSL_IMX7_NUM_PWMS] = {
> +            FSL_IMX7_PWM1_ADDR,
> +            FSL_IMX7_PWM2_ADDR,
> +            FSL_IMX7_PWM3_ADDR,
> +            FSL_IMX7_PWM4_ADDR,
> +        };
> +
> +        snprintf(name, NAME_SIZE, "pwm%d", i);
> +        create_unimplemented_device(name, FSL_IMX7_PWMn_ADDR[i],
> +                                    FSL_IMX7_PWMn_SIZE);
> +    }
>
>      /*
> -     * SAI (Audio SSI (Synchronous Serial Interface))
> +     * CANs
>       */
> -    create_unimplemented_device("sai1", FSL_IMX7_SAI1_ADDR, 
> FSL_IMX7_SAIn_SIZE);
> -    create_unimplemented_device("sai2", FSL_IMX7_SAI2_ADDR, 
> FSL_IMX7_SAIn_SIZE);
> -    create_unimplemented_device("sai2", FSL_IMX7_SAI3_ADDR, 
> FSL_IMX7_SAIn_SIZE);
> +    for (i = 0; i < FSL_IMX7_NUM_CANS; i++) {
> +        static const hwaddr FSL_IMX7_CANn_ADDR[FSL_IMX7_NUM_CANS] = {
> +            FSL_IMX7_CAN1_ADDR,
> +            FSL_IMX7_CAN2_ADDR,
> +        };
> +
> +        snprintf(name, NAME_SIZE, "can%d", i);
> +        create_unimplemented_device(name, FSL_IMX7_CANn_ADDR[i],
> +                                    FSL_IMX7_CANn_SIZE);
> +    }


thanks
-- PMM