Re: [PATCH v3 1/5] Refactor i.MX6UL processor code

[Jean-Christophe DUBOIS]

Le 02/08/2023 à 23:32, Philippe Mathieu-Daudé a écrit :
> Hi Jean-Christophe,
>
> On 2/8/23 23:08, Jean-Christophe Dubois wrote:
> > * Add Addr and size definition for all i.MX6UL devices in i.MX6UL 
> > header file.
>
> I'm OK with your patch, but some addr/size are added, while other
> are changed. It is hard to review. Having one patch for changes
> and another for additions would help review.

I tried to set addresses and sizes following the order set for devices 
in the i.MX6UL reference manual. I found it easier to follow then (and 
make reasonably sure I didn't miss some).

I certainly understand that the reorder is annoying for the review. I 
can try to do as you intended but I am not sure the reorder review would 
be really easier.

> > * Use those newly defined named constants whenever possible.
> > * Standardize the way we init a familly of unimplemented devices
> >    - SAI
> >    - PWM (add missing PWM instances)
> >    - CAN
> > * Add/rework few comments
> >
> > Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
> > ---
> >   hw/arm/fsl-imx6ul.c         | 149 +++++++++++++++++++++++------------
> >   include/hw/arm/fsl-imx6ul.h | 150 +++++++++++++++++++++++++++++++++---
> >   2 files changed, 240 insertions(+), 59 deletions(-)
>
>
> > diff --git a/include/hw/arm/fsl-imx6ul.h b/include/hw/arm/fsl-imx6ul.h
> > index 9ee15ae38d..5d381740ef 100644
> > --- a/include/hw/arm/fsl-imx6ul.h
> > +++ b/include/hw/arm/fsl-imx6ul.h
>
> For example here:
>
> > +    FSL_IMX6UL_SNVS_HP_SIZE         = (4 * KiB),
> > +
> >       FSL_IMX6UL_USBPHY2_ADDR         = 0x020CA000,
> > -    FSL_IMX6UL_USBPHY2_SIZE         = (4 * 1024),
>
> > -    FSL_IMX6UL_USBPHY1_SIZE         = (4 * 1024),
> > +    FSL_IMX6UL_USBPHYn_SIZE         = 0x100,
>
> Don't we also need:

Well, I did not modify the i.MX USB PHY file by itself. It is a fact 
that the last i.MX USB PHY register is at 0x80 offset and a 0x1000 
memory region for the device is certainly oversized even if the 
processor memory map is actually provisioning a 0x1000 address space 
between distinct USB PHY devices.  My intent in lowering the device 
register region size as close to the real size as possible was that in 
case a device was not "implemented" in Qemu we could just map it as an 
unimplemented device (allowing dummy access to the register range) but 
get some kind of platform "bus error" if the software was trying to 
access some "registers" in the upper part of the memory region (as you 
would on the real hardware?).

So 0x1000 is not wrong per se as the USB phy device implementation code 
is logging the illegal access when software is doing access over 0x80 
offset. This would just not trigger a processor hardware access fault 
(when it could/should?).

> -- >8 --
> --- a/hw/usb/imx-usb-phy.c
> +++ b/hw/usb/imx-usb-phy.c
> @@ -210,7 +210,7 @@ static void imx_usbphy_realize(DeviceState *dev, 
> Error **errp)
>      IMXUSBPHYState *s = IMX_USBPHY(dev);
>
>      memory_region_init_io(&s->iomem, OBJECT(s), &imx_usbphy_ops, s,
> -                          "imx-usbphy", 0x1000);
> +                          "imx-usbphy", 0x100);
>      sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
>  }
>
> ---
>
> ?
>
> Thanks,
>
> Phil.