Re: [PATCH 3/4] lib: logic_pio: Introduce MMIO_LOWER_RESERVED

[John Garry]

On 27/04/2020 12:03, Jiaxun Yang wrote:
> ä 2020å4æ27æ GMT+08:00 äå6:43:09, John Garry <john.garry@xxxxxxxxxx> åå:
> > On 26/04/2020 12:47, Jiaxun Yang wrote:
> > > That would allow platforms reserve some lower address in PIO MMIO range
> > > to deal with legacy drivers with hardcoded I/O ports that can't be
> > > managed by logic_pio.
> >
> > Hi,
> >
> > Is there some reason why the logic_pio code cannot be improved to handle
> > these devices at these "fixed" addresses? Or do you have a plan to
> > improve it? We already support fixed bus address devices in the INDIRECT
> > IO region.
>
> Hi,
>
> The issue about "Fixed Address" is we can't control the ioport
> That driver used to operate devices.
> So any attempt to resolve it in logic_pio seems impossible.
>
> Currently we have i8259, i8042, piix4_smbus, mc146818 rely on this assumption.

Right, and from glancing at a couple of drivers you mentioned, if we 
were to register a logic pio region for that legacy region, there does 
not seem to be an easy place to fixup to use logic pio addresses (for 
those devices). They use hardcoded values. However if all those drivers 
were mips specific, you could fixup those drivers to use logic_pio 
addresses today through some macro. But not sure on that.

So, going back to your change, I have a dilemma wondering whether you 
should still register a logic pio region for the legacy region instead 
of the carveout reservation, but ensure it is the first region 
registered, such that logic pio address base is 0 and no translation is 
required. At least then you have a region registered and it shows in 
/proc/ioports, but then this whole thing becomes a bit fragile.

Maybe Arnd or Bjorn have an opinion on this.

Thanks,
John


> My plan is after getting this part merged, I'm going to work on a ISA Host bridge driver,
> then convert device drivers into logic_pio and Devicetree based driver step by step.
>
> Finally when we nologner have any legacy driver, we can safely remove this reserved
> range.
>
> Thanks.
>
>
> > Carving out a region of IO space is less than ideal.
> >
> > Thanks,
> > John
> >
> > > Signed-off-by: Jiaxun Yang <jiaxun.yang@xxxxxxxxxxx>
> > > ---
> > >    lib/logic_pio.c | 6 +++++-
> > >    1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/lib/logic_pio.c b/lib/logic_pio.c
> > > index f511a99bb389..57fff1cb7063 100644
> > > --- a/lib/logic_pio.c
> > > +++ b/lib/logic_pio.c
> > > @@ -20,6 +20,10 @@
> > >    static LIST_HEAD(io_range_list);
> > >    static DEFINE_MUTEX(io_range_mutex);
> > >    
> > > +#ifndef MMIO_LOWER_RESERVED
> > > +#define MMIO_LOWER_RESERVED	0
> > > +#endif
> > > +
> > >    /* Consider a kernel general helper for this */
> > >    #define in_range(b, first, len)        ((b) >= (first) && (b) < (first) + (len))
> > >    
> > > @@ -36,7 +40,7 @@ int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
> > >    	struct logic_pio_hwaddr *range;
> > >    	resource_size_t start;
> > >    	resource_size_t end;
> > > -	resource_size_t mmio_end = 0;
> > > +	resource_size_t mmio_end = MMIO_LOWER_RESERVED;
> > >    	resource_size_t iio_sz = MMIO_UPPER_LIMIT;
> > >    	int ret = 0;
> > >