subject:"\[edk2\] \[PATCH 1\/1\] MdePkg\/IoLib\: Add support for big\-endian MMIO"

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

2017-12-04 Thread Kinney, Michael D

Sounds good.

Mike

> -Original Message-
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> Sent: Monday, December 4, 2017 7:54 AM
> To: Kinney, Michael D <michael.d.kin...@intel.com>
> Cc: Udit Kumar <udit.ku...@nxp.com>; Meenakshi Aggarwal
> <meenakshi.aggar...@nxp.com>; Varun Sethi
> <v.se...@nxp.com>; edk2-devel@lists.01.org; Gao, Liming
> <liming@intel.com>; Ard Biesheuvel
> <ard.biesheu...@linaro.org>
> Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support
> for big-endian MMIO
> 
> Hi Mike,
> 
> This separate library would only be necessary
> specifically because
> an additional library to the default platform IoLib was
> needed.
> So while it's another dependency, it would likely reduce
> image size.
> 
> Which is why I was leaning towards a wrapper.
> 
> This also means the actual hw-dependent bit gets
> abstracted away from
> the portable and predictable byteswapping. Which always
> makes me
> slightly more comfortable.
> 
> If you're OK with the concept, I can throw an RFC
> together.
> 
> Best Regards,
> 
> Leif
> 
> On Mon, Dec 04, 2017 at 03:31:10PM +, Kinney, Michael
> D wrote:
> > Leif,
> >
> > I may make more sense to add to MdePkg as a peer to
> IoLib.
> > This minimizes the package dependencies for Si modules.
> >
> > I am ok as a wrapper on top of IoLib.  Whatever reduces
> > code duplication and reduces maintenance.
> >
> > Best regards,
> >
> > Mike
> >
> > > -Original Message-
> > > From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> > > Sent: Monday, December 4, 2017 4:36 AM
> > > To: Kinney, Michael D <michael.d.kin...@intel.com>
> > > Cc: Udit Kumar <udit.ku...@nxp.com>; Meenakshi
> Aggarwal
> > > <meenakshi.aggar...@nxp.com>; Varun Sethi
> > > <v.se...@nxp.com>; edk2-devel@lists.01.org; Gao,
> Liming
> > > <liming@intel.com>; Ard Biesheuvel
> > > <ard.biesheu...@linaro.org>
> > > Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add
> support
> > > for big-endian MMIO
> > >
> > > Mike,
> > >
> > > In that case - do you think it should be added to
> > > MdeModulePkg?
> > >
> > > Should it be implemented simply as a wrapper on IoLib
> > > (depending on
> > > it)?
> > >
> > > /
> > > Leif
> > >
> > > On Fri, Dec 01, 2017 at 10:41:26PM +, Kinney,
> Michael
> > > D wrote:
> > > > Udit,
> > > >
> > > > I agree with your concern.
> > > >
> > > > I am in favor of adding new APIs that perform the
> > > > byte swap operations.
> > > >
> > > > Mike
> > > >
> > > > > -Original Message-
> > > > > From: Udit Kumar [mailto:udit.ku...@nxp.com]
> > > > > Sent: Friday, December 1, 2017 9:58 AM
> > > > > To: Leif Lindholm <leif.lindh...@linaro.org>;
> > > Meenakshi
> > > > > Aggarwal <meenakshi.aggar...@nxp.com>; Varun
> Sethi
> > > > > <v.se...@nxp.com>
> > > > > Cc: Kinney, Michael D
> <michael.d.kin...@intel.com>;
> > > edk2-
> > > > > de...@lists.01.org; Gao, Liming
> > > <liming@intel.com>;
> > > > > Ard Biesheuvel <ard.biesheu...@linaro.org>
> > > > > Subject: RE: [edk2] [PATCH 1/1] MdePkg/IoLib: Add
> > > support
> > > > > for big-endian MMIO
> > > > >
> > > > > Thanks Leif,
> > > > >
> > > > > > It may require a little bit more of up-front
> work,
> > > but
> > > > > the end result will be a
> > > > > > platform port that works with the intended edk2
> > > design
> > > > > principles rather than
> > > > >
> > > > > Yes, this will be re-design/code for us, breaking
> all
> > > > > software pieces into
> > > > > smaller blocks and exposing protocol from the
> same.
> > > > > This could be managed.
> > > > >
> > > > > But how do you see,  if there is such dependency
> oN
> > > lib.
> > > > > Say a driver which is in BE mode, and is using
> > > DebugLib
> > > > > (BaseDebugLibSerialPort)
> > > > > And DebugLib uses SerialPortLib, which is in LE
> mode
> > > > >

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

2017-12-04 Thread Leif Lindholm

Hi Mike,

This separate library would only be necessary specifically because
an additional library to the default platform IoLib was needed.
So while it's another dependency, it would likely reduce image size.

Which is why I was leaning towards a wrapper.

This also means the actual hw-dependent bit gets abstracted away from
the portable and predictable byteswapping. Which always makes me
slightly more comfortable.

If you're OK with the concept, I can throw an RFC together.

Best Regards,

Leif

On Mon, Dec 04, 2017 at 03:31:10PM +, Kinney, Michael D wrote:
> Leif,
> 
> I may make more sense to add to MdePkg as a peer to IoLib.
> This minimizes the package dependencies for Si modules.
> 
> I am ok as a wrapper on top of IoLib.  Whatever reduces 
> code duplication and reduces maintenance.
> 
> Best regards,
> 
> Mike
> 
> > -Original Message-
> > From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> > Sent: Monday, December 4, 2017 4:36 AM
> > To: Kinney, Michael D <michael.d.kin...@intel.com>
> > Cc: Udit Kumar <udit.ku...@nxp.com>; Meenakshi Aggarwal
> > <meenakshi.aggar...@nxp.com>; Varun Sethi
> > <v.se...@nxp.com>; edk2-devel@lists.01.org; Gao, Liming
> > <liming....@intel.com>; Ard Biesheuvel
> > <ard.biesheu...@linaro.org>
> > Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support
> > for big-endian MMIO
> > 
> > Mike,
> > 
> > In that case - do you think it should be added to
> > MdeModulePkg?
> > 
> > Should it be implemented simply as a wrapper on IoLib
> > (depending on
> > it)?
> > 
> > /
> > Leif
> > 
> > On Fri, Dec 01, 2017 at 10:41:26PM +, Kinney, Michael
> > D wrote:
> > > Udit,
> > >
> > > I agree with your concern.
> > >
> > > I am in favor of adding new APIs that perform the
> > > byte swap operations.
> > >
> > > Mike
> > >
> > > > -Original Message-
> > > > From: Udit Kumar [mailto:udit.ku...@nxp.com]
> > > > Sent: Friday, December 1, 2017 9:58 AM
> > > > To: Leif Lindholm <leif.lindh...@linaro.org>;
> > Meenakshi
> > > > Aggarwal <meenakshi.aggar...@nxp.com>; Varun Sethi
> > > > <v.se...@nxp.com>
> > > > Cc: Kinney, Michael D <michael.d.kin...@intel.com>;
> > edk2-
> > > > de...@lists.01.org; Gao, Liming
> > <liming@intel.com>;
> > > > Ard Biesheuvel <ard.biesheu...@linaro.org>
> > > > Subject: RE: [edk2] [PATCH 1/1] MdePkg/IoLib: Add
> > support
> > > > for big-endian MMIO
> > > >
> > > > Thanks Leif,
> > > >
> > > > > It may require a little bit more of up-front work,
> > but
> > > > the end result will be a
> > > > > platform port that works with the intended edk2
> > design
> > > > principles rather than
> > > >
> > > > Yes, this will be re-design/code for us, breaking all
> > > > software pieces into
> > > > smaller blocks and exposing protocol from the same.
> > > > This could be managed.
> > > >
> > > > But how do you see,  if there is such dependency oN
> > lib.
> > > > Say a driver which is in BE mode, and is using
> > DebugLib
> > > > (BaseDebugLibSerialPort)
> > > > And DebugLib uses SerialPortLib, which is in LE mode
> > > >
> > > > Thanks
> > > > Udit
> > > >
> > > > > -Original Message-
> > > > > From: edk2-devel [mailto:edk2-devel-
> > > > boun...@lists.01.org] On Behalf Of Leif
> > > > > Lindholm
> > > > > Sent: Friday, December 01, 2017 4:28 PM
> > > > > To: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>
> > > > > Cc: Kinney, Michael D <michael.d.kin...@intel.com>;
> > > > edk2-devel@lists.01.org;
> > > > > Gao, Liming <liming@intel.com>; Ard Biesheuvel
> > > > > <ard.biesheu...@linaro.org>
> > > > > Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add
> > > > support for big-endian
> > > > > MMIO
> > > > >
> > > > > On Thu, Nov 30, 2017 at 04:15:38AM +, Meenakshi
> > > > Aggarwal wrote:
> > > > > > Hi Leif, Mike,
> > > > > >
> > > > > >
> > > > > > NXP boards, at present, have few controllers with
> > big
>

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

2017-12-04 Thread Leif Lindholm

Mike,

In that case - do you think it should be added to MdeModulePkg?

Should it be implemented simply as a wrapper on IoLib (depending on
it)?

/
Leif

On Fri, Dec 01, 2017 at 10:41:26PM +, Kinney, Michael D wrote:
> Udit,
> 
> I agree with your concern.
> 
> I am in favor of adding new APIs that perform the
> byte swap operations.
> 
> Mike
> 
> > -Original Message-
> > From: Udit Kumar [mailto:udit.ku...@nxp.com]
> > Sent: Friday, December 1, 2017 9:58 AM
> > To: Leif Lindholm <leif.lindh...@linaro.org>; Meenakshi
> > Aggarwal <meenakshi.aggar...@nxp.com>; Varun Sethi
> > <v.se...@nxp.com>
> > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; edk2-
> > de...@lists.01.org; Gao, Liming <liming@intel.com>;
> > Ard Biesheuvel <ard.biesheu...@linaro.org>
> > Subject: RE: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support
> > for big-endian MMIO
> > 
> > Thanks Leif,
> > 
> > > It may require a little bit more of up-front work, but
> > the end result will be a
> > > platform port that works with the intended edk2 design
> > principles rather than
> > 
> > Yes, this will be re-design/code for us, breaking all
> > software pieces into
> > smaller blocks and exposing protocol from the same.
> > This could be managed.
> > 
> > But how do you see,  if there is such dependency oN lib.
> > Say a driver which is in BE mode, and is using DebugLib
> > (BaseDebugLibSerialPort)
> > And DebugLib uses SerialPortLib, which is in LE mode
> > 
> > Thanks
> > Udit
> > 
> > > -Original Message-
> > > From: edk2-devel [mailto:edk2-devel-
> > boun...@lists.01.org] On Behalf Of Leif
> > > Lindholm
> > > Sent: Friday, December 01, 2017 4:28 PM
> > > To: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>
> > > Cc: Kinney, Michael D <michael.d.kin...@intel.com>;
> > edk2-devel@lists.01.org;
> > > Gao, Liming <liming@intel.com>; Ard Biesheuvel
> > > <ard.biesheu...@linaro.org>
> > > Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add
> > support for big-endian
> > > MMIO
> > >
> > > On Thu, Nov 30, 2017 at 04:15:38AM +, Meenakshi
> > Aggarwal wrote:
> > > > Hi Leif, Mike,
> > > >
> > > >
> > > > NXP boards, at present, have few controllers with big
> > endian and other
> > > > with little endian memory access.
> > >
> > > Sure, this is not a problem.
> > >
> > > > Maximum controllers depend on SocLib library for
> > clock information and
> > > > endianness for SocLib and controllers is different.
> > >
> > > OK, I see in SocLib you have something called a Gur
> > that is read once (and again
> > > does a special per-device Pcd dance for runtime
> > selection between
> > > byteswapping Mmio and plain Mmio).
> > >
> > > > So this option will not work for us,
> > > >   You would then just do (in your .dsc):
> > > >   Platform/NXP/Drivers/WatchDog/WatchDogDxe.inf {
> > > >   IoLib|
> > > >   }
> > > >
> > > > Because all libraries included by WatchDog will then
> > access BE mode Mmio
> > > APIs.
> > >
> > > Which libraries performing Mmio accesses are you
> > intending to include in your
> > > watchdog driver? You have submitted none as part of
> > this series.
> > >
> > > > And breaking code into separate modules with
> > different access will be
> > > > very difficult.
> > >
> > > It may require a little bit more of up-front work, but
> > the end result will be a
> > > platform port that works with the intended edk2 design
> > principles rather than
> > > against them. And that will reduce the overall effort
> > (not to mention code
> > > duplication).
> > >
> > > From the patches you have sent, the only required
> > change I see (if a
> > > byteswapping IoLib was added to edk2) would be to
> > create a tiny driver for this
> > > "Gur" device that installs a protocol containing a
> > single function for reading
> > > from that device's register space. That driver can be
> > built against the swapping
> > > or non-swapping IoLib as appropriate.
> > >
> > > > Watchdog is not the only module which need BE Mmio
> > APIs, we have MMC
> > > > and other con

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

2017-12-01 Thread Kinney, Michael D

Udit,

I agree with your concern.

I am in favor of adding new APIs that perform the
byte swap operations.

Mike

> -Original Message-
> From: Udit Kumar [mailto:udit.ku...@nxp.com]
> Sent: Friday, December 1, 2017 9:58 AM
> To: Leif Lindholm <leif.lindh...@linaro.org>; Meenakshi
> Aggarwal <meenakshi.aggar...@nxp.com>; Varun Sethi
> <v.se...@nxp.com>
> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; edk2-
> de...@lists.01.org; Gao, Liming <liming@intel.com>;
> Ard Biesheuvel <ard.biesheu...@linaro.org>
> Subject: RE: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support
> for big-endian MMIO
> 
> Thanks Leif,
> 
> > It may require a little bit more of up-front work, but
> the end result will be a
> > platform port that works with the intended edk2 design
> principles rather than
> 
> Yes, this will be re-design/code for us, breaking all
> software pieces into
> smaller blocks and exposing protocol from the same.
> This could be managed.
> 
> But how do you see,  if there is such dependency oN lib.
> Say a driver which is in BE mode, and is using DebugLib
> (BaseDebugLibSerialPort)
> And DebugLib uses SerialPortLib, which is in LE mode
> 
> Thanks
> Udit
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-
> boun...@lists.01.org] On Behalf Of Leif
> > Lindholm
> > Sent: Friday, December 01, 2017 4:28 PM
> > To: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>
> > Cc: Kinney, Michael D <michael.d.kin...@intel.com>;
> edk2-devel@lists.01.org;
> > Gao, Liming <liming@intel.com>; Ard Biesheuvel
> > <ard.biesheu...@linaro.org>
> > Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add
> support for big-endian
> > MMIO
> >
> > On Thu, Nov 30, 2017 at 04:15:38AM +, Meenakshi
> Aggarwal wrote:
> > > Hi Leif, Mike,
> > >
> > >
> > > NXP boards, at present, have few controllers with big
> endian and other
> > > with little endian memory access.
> >
> > Sure, this is not a problem.
> >
> > > Maximum controllers depend on SocLib library for
> clock information and
> > > endianness for SocLib and controllers is different.
> >
> > OK, I see in SocLib you have something called a Gur
> that is read once (and again
> > does a special per-device Pcd dance for runtime
> selection between
> > byteswapping Mmio and plain Mmio).
> >
> > > So this option will not work for us,
> > >   You would then just do (in your .dsc):
> > >   Platform/NXP/Drivers/WatchDog/WatchDogDxe.inf {
> > >   IoLib|
> > >   }
> > >
> > > Because all libraries included by WatchDog will then
> access BE mode Mmio
> > APIs.
> >
> > Which libraries performing Mmio accesses are you
> intending to include in your
> > watchdog driver? You have submitted none as part of
> this series.
> >
> > > And breaking code into separate modules with
> different access will be
> > > very difficult.
> >
> > It may require a little bit more of up-front work, but
> the end result will be a
> > platform port that works with the intended edk2 design
> principles rather than
> > against them. And that will reduce the overall effort
> (not to mention code
> > duplication).
> >
> > From the patches you have sent, the only required
> change I see (if a
> > byteswapping IoLib was added to edk2) would be to
> create a tiny driver for this
> > "Gur" device that installs a protocol containing a
> single function for reading
> > from that device's register space. That driver can be
> built against the swapping
> > or non-swapping IoLib as appropriate.
> >
> > > Watchdog is not the only module which need BE Mmio
> APIs, we have MMC
> > > and other controllers also with same requirement.
> >
> > And the same solutions are possible everywhere.
> >
> > Best Regards,
> >
> > Leif
> >
> > > We need BE Mmio APIs with a different name.
> > >
> > > Please see the possibility.
> > >
> > > Thanks & Regards,
> > > Meenakshi
> > >
> > > > -Original Message-
> > > > From: Leif Lindholm
> [mailto:leif.lindh...@linaro.org]
> > > > Sent: Thursday, November 30, 2017 1:19 AM
> > > > To: Kinney, Michael D <michael.d.kin...@intel.com>
> > > > Cc: Meenakshi Aggarwal
> <meenakshi.aggar...@nxp.com>; Gao, Liming
> > > > <liming@intel.com>; edk2-devel

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

2017-12-01 Thread Udit Kumar

Thanks Leif, 

> It may require a little bit more of up-front work, but the end result will be 
> a
> platform port that works with the intended edk2 design principles rather than

Yes, this will be re-design/code for us, breaking all software pieces into
smaller blocks and exposing protocol from the same. 
This could be managed.

But how do you see,  if there is such dependency oN lib. 
Say a driver which is in BE mode, and is using DebugLib (BaseDebugLibSerialPort)
And DebugLib uses SerialPortLib, which is in LE mode 

Thanks 
Udit

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Leif
> Lindholm
> Sent: Friday, December 01, 2017 4:28 PM
> To: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>
> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; edk2-devel@lists.01.org;
> Gao, Liming <liming@intel.com>; Ard Biesheuvel
> <ard.biesheu...@linaro.org>
> Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian
> MMIO
> 
> On Thu, Nov 30, 2017 at 04:15:38AM +, Meenakshi Aggarwal wrote:
> > Hi Leif, Mike,
> >
> >
> > NXP boards, at present, have few controllers with big endian and other
> > with little endian memory access.
> 
> Sure, this is not a problem.
> 
> > Maximum controllers depend on SocLib library for clock information and
> > endianness for SocLib and controllers is different.
> 
> OK, I see in SocLib you have something called a Gur that is read once (and 
> again
> does a special per-device Pcd dance for runtime selection between
> byteswapping Mmio and plain Mmio).
> 
> > So this option will not work for us,
> >   You would then just do (in your .dsc):
> >   Platform/NXP/Drivers/WatchDog/WatchDogDxe.inf {
> >   IoLib|
> >   }
> >
> > Because all libraries included by WatchDog will then access BE mode Mmio
> APIs.
> 
> Which libraries performing Mmio accesses are you intending to include in your
> watchdog driver? You have submitted none as part of this series.
> 
> > And breaking code into separate modules with different access will be
> > very difficult.
> 
> It may require a little bit more of up-front work, but the end result will be 
> a
> platform port that works with the intended edk2 design principles rather than
> against them. And that will reduce the overall effort (not to mention code
> duplication).
> 
> From the patches you have sent, the only required change I see (if a
> byteswapping IoLib was added to edk2) would be to create a tiny driver for 
> this
> "Gur" device that installs a protocol containing a single function for reading
> from that device's register space. That driver can be built against the 
> swapping
> or non-swapping IoLib as appropriate.
> 
> > Watchdog is not the only module which need BE Mmio APIs, we have MMC
> > and other controllers also with same requirement.
> 
> And the same solutions are possible everywhere.
> 
> Best Regards,
> 
> Leif
> 
> > We need BE Mmio APIs with a different name.
> >
> > Please see the possibility.
> >
> > Thanks & Regards,
> > Meenakshi
> >
> > > -Original Message-
> > > From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> > > Sent: Thursday, November 30, 2017 1:19 AM
> > > To: Kinney, Michael D <michael.d.kin...@intel.com>
> > > Cc: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>; Gao, Liming
> > > <liming@intel.com>; edk2-devel@lists.01.org; Ard Biesheuvel
> > > <ard.biesheu...@linaro.org>
> > > Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for
> > > big-endian MMIO
> > >
> > > I guess there is no strict rule about a driver only directly
> > > accessing one piece of HW?
> > >
> > > Still, that would be one possible solution: breaking accesses to a
> > > separate HW in need of byteswapping out into its own module and
> > > letting it override IoLib version there.
> > >
> > > Regards,
> > >
> > > Leif
> > >
> > > On Wed, Nov 29, 2017 at 07:25:05PM +, Kinney, Michael D wrote:
> > > > Leif,
> > > >
> > > > I agree that this should be described as byte swapping.
> > > >
> > > > What about a module that needs to access HW with and without the
> > > > bytes swapped?  It gets more complex if a module is linked against
> > > > several libs and some libs access HW with bytes swapped and some
> > > > do not.
> > > >
> > > > Thanks,
> > >

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

2017-12-01 Thread Leif Lindholm

On Thu, Nov 30, 2017 at 04:15:38AM +, Meenakshi Aggarwal wrote:
> Hi Leif, Mike,
> 
> 
> NXP boards, at present, have few controllers with big endian and
> other with little endian memory access.

Sure, this is not a problem.

> Maximum controllers depend on SocLib library for clock information
> and endianness for SocLib and controllers is different.

OK, I see in SocLib you have something called a Gur that is read once
(and again does a special per-device Pcd dance for runtime selection
between byteswapping Mmio and plain Mmio).

> So this option will not work for us,
>   You would then just do (in your .dsc):
>   Platform/NXP/Drivers/WatchDog/WatchDogDxe.inf {
>   IoLib|
>   }
> 
> Because all libraries included by WatchDog will then access BE mode Mmio APIs.

Which libraries performing Mmio accesses are you intending to include
in your watchdog driver? You have submitted none as part of this series.

> And breaking code into separate modules with different access will
> be very difficult.

It may require a little bit more of up-front work, but the end result
will be a platform port that works with the intended edk2 design
principles rather than against them. And that will reduce the overall
effort (not to mention code duplication).

From the patches you have sent, the only required change I see (if a
byteswapping IoLib was added to edk2) would be to create a tiny driver
for this "Gur" device that installs a protocol containing a single
function for reading from that device's register space. That driver
can be built against the swapping or non-swapping IoLib as
appropriate.

> Watchdog is not the only module which need BE Mmio APIs, we have MMC
> and other controllers also with same requirement.

And the same solutions are possible everywhere.

Best Regards,

Leif

> We need BE Mmio APIs with a different name.
> 
> Please see the possibility.
>
> Thanks & Regards,
> Meenakshi
> 
> > -Original Message-
> > From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> > Sent: Thursday, November 30, 2017 1:19 AM
> > To: Kinney, Michael D <michael.d.kin...@intel.com>
> > Cc: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>; Gao, Liming
> > <liming....@intel.com>; edk2-devel@lists.01.org; Ard Biesheuvel
> > <ard.biesheu...@linaro.org>
> > Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian
> > MMIO
> > 
> > I guess there is no strict rule about a driver only directly accessing
> > one piece of HW?
> > 
> > Still, that would be one possible solution: breaking accesses to a
> > separate HW in need of byteswapping out into its own module and
> > letting it override IoLib version there.
> > 
> > Regards,
> > 
> > Leif
> > 
> > On Wed, Nov 29, 2017 at 07:25:05PM +, Kinney, Michael D wrote:
> > > Leif,
> > >
> > > I agree that this should be described as byte swapping.
> > >
> > > What about a module that needs to access HW with and
> > > without the bytes swapped?  It gets more complex if a
> > > module is linked against several libs and some libs
> > > access HW with bytes swapped and some do not.
> > >
> > > Thanks,
> > >
> > > Mike
> > >
> > > > -Original Message-
> > > > From: edk2-devel [mailto:edk2-devel-
> > > > boun...@lists.01.org] On Behalf Of Leif Lindholm
> > > > Sent: Wednesday, November 29, 2017 4:51 AM
> > > > To: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>;
> > > > Gao, Liming <liming@intel.com>
> > > > Cc: Kinney, Michael D <michael.d.kin...@intel.com>;
> > > > edk2-devel@lists.01.org; Ard Biesheuvel
> > > > <ard.biesheu...@linaro.org>
> > > > Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add
> > > > support for big-endian MMIO
> > > >
> > > > Hi Meenakshi,
> > > >
> > > > I finally got around to looking at the watchdog code
> > > > (that uses this
> > > > library), and that has convinced me the best solution
> > > > is to do what
> > > > Liming proposed.
> > > >
> > > > Looking at this snippet:
> > > >
> > > > > +STATIC
> > > > > +UINT16
> > > > > +EFIAPI
> > > > > +WdogRead (
> > > > > +  IN  UINTN     Address
> > > > > +  )
> > > > > +{
> > > > > +  if (FixedPcdGetBool (PcdWdogBigEndian)) {
> > > > > +    return BeMmioRead16 (Address);
> > > > > +  } else

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

2017-11-29 Thread Meenakshi Aggarwal

Hi Leif, Mike,


NXP boards, at present, have few controllers with big endian and other with 
little endian memory access.

Maximum controllers depend on SocLib library for clock information and 
endianness for SocLib and
controllers is different.

So this option will not work for us,
  You would then just do (in your .dsc):
  Platform/NXP/Drivers/WatchDog/WatchDogDxe.inf {
  IoLib|
  }

Because all libraries included by WatchDog will then access BE mode Mmio APIs.


And breaking code into separate modules with different access will be very 
difficult.
Watchdog is not the only module which need BE Mmio APIs, we have MMC and other 
controllers also with same requirement.

We need BE Mmio APIs with a different name.

Please see the possibility.


Thanks & Regards,
Meenakshi

> -Original Message-
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> Sent: Thursday, November 30, 2017 1:19 AM
> To: Kinney, Michael D <michael.d.kin...@intel.com>
> Cc: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>; Gao, Liming
> <liming@intel.com>; edk2-devel@lists.01.org; Ard Biesheuvel
> <ard.biesheu...@linaro.org>
> Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian
> MMIO
> 
> I guess there is no strict rule about a driver only directly accessing
> one piece of HW?
> 
> Still, that would be one possible solution: breaking accesses to a
> separate HW in need of byteswapping out into its own module and
> letting it override IoLib version there.
> 
> Regards,
> 
> Leif
> 
> On Wed, Nov 29, 2017 at 07:25:05PM +, Kinney, Michael D wrote:
> > Leif,
> >
> > I agree that this should be described as byte swapping.
> >
> > What about a module that needs to access HW with and
> > without the bytes swapped?  It gets more complex if a
> > module is linked against several libs and some libs
> > access HW with bytes swapped and some do not.
> >
> > Thanks,
> >
> > Mike
> >
> > > -Original Message-
> > > From: edk2-devel [mailto:edk2-devel-
> > > boun...@lists.01.org] On Behalf Of Leif Lindholm
> > > Sent: Wednesday, November 29, 2017 4:51 AM
> > > To: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>;
> > > Gao, Liming <liming....@intel.com>
> > > Cc: Kinney, Michael D <michael.d.kin...@intel.com>;
> > > edk2-devel@lists.01.org; Ard Biesheuvel
> > > <ard.biesheu...@linaro.org>
> > > Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add
> > > support for big-endian MMIO
> > >
> > > Hi Meenakshi,
> > >
> > > I finally got around to looking at the watchdog code
> > > (that uses this
> > > library), and that has convinced me the best solution
> > > is to do what
> > > Liming proposed.
> > >
> > > Looking at this snippet:
> > >
> > > > +STATIC
> > > > +UINT16
> > > > +EFIAPI
> > > > +WdogRead (
> > > > +  IN  UINTN     Address
> > > > +  )
> > > > +{
> > > > +  if (FixedPcdGetBool (PcdWdogBigEndian)) {
> > > > +    return BeMmioRead16 (Address);
> > > > +  } else {
> > > > +    return MmioRead16(Address);
> > > > +  }
> > >
> > > This is actually a pretty good demonstration of the
> > > arguments that
> > > were made with regards to how the BeIoLib could be just
> > > another
> > > implementation of IoLib.
> > >
> > > You would then just do (in your .dsc):
> > >   Platform/NXP/Drivers/WatchDog/WatchDogDxe.inf {
> > >     IoLib|
> > >   }
> > >
> > > This causes the big-endian version of the library to be
> > > used for this
> > > component. This makes these Wdog functions and
> > > the Pcd
> > > redundant, and the rest of the code can use
> > > MmioRead16()/MmioWrite16()
> > > directly.
> > >
> > > But then, it is not really a big-endian or litte-endian
> > > version of the
> > > library we need. We always know which endianness we are
> > > building for.
> > > What we need is a byteswapping flavour of IoLib.
> > >
> > > So Liming, what if we do something like adding a
> > > MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSwa
> > > p.inf
> > > ---
> > > [Defines]
> > >   INF_VERSION= 0x0001001A
> > >   BASE_NAME  =
> > > BaseIoLibIntrinsicSwap
> > >   MODULE_UNI_FILE=
> > >

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

2017-11-29 Thread Leif Lindholm

I guess there is no strict rule about a driver only directly accessing
one piece of HW?

Still, that would be one possible solution: breaking accesses to a
separate HW in need of byteswapping out into its own module and
letting it override IoLib version there.

Regards,

Leif

On Wed, Nov 29, 2017 at 07:25:05PM +, Kinney, Michael D wrote:
> Leif,
> 
> I agree that this should be described as byte swapping.
> 
> What about a module that needs to access HW with and
> without the bytes swapped?  It gets more complex if a
> module is linked against several libs and some libs
> access HW with bytes swapped and some do not.
> 
> Thanks,
> 
> Mike
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-
> > boun...@lists.01.org] On Behalf Of Leif Lindholm
> > Sent: Wednesday, November 29, 2017 4:51 AM
> > To: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>;
> > Gao, Liming <liming@intel.com>
> > Cc: Kinney, Michael D <michael.d.kin...@intel.com>;
> > edk2-devel@lists.01.org; Ard Biesheuvel
> > <ard.biesheu...@linaro.org>
> > Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add
> > support for big-endian MMIO
> > 
> > Hi Meenakshi,
> > 
> > I finally got around to looking at the watchdog code
> > (that uses this
> > library), and that has convinced me the best solution
> > is to do what
> > Liming proposed.
> > 
> > Looking at this snippet:
> > 
> > > +STATIC
> > > +UINT16
> > > +EFIAPI
> > > +WdogRead (
> > > +  IN  UINTN     Address
> > > +  )
> > > +{
> > > +  if (FixedPcdGetBool (PcdWdogBigEndian)) {
> > > +    return BeMmioRead16 (Address);
> > > +  } else {
> > > +    return MmioRead16(Address);
> > > +  }
> > 
> > This is actually a pretty good demonstration of the
> > arguments that
> > were made with regards to how the BeIoLib could be just
> > another
> > implementation of IoLib.
> > 
> > You would then just do (in your .dsc):
> >   Platform/NXP/Drivers/WatchDog/WatchDogDxe.inf {
> >     IoLib|
> >   }
> > 
> > This causes the big-endian version of the library to be
> > used for this
> > component. This makes these Wdog functions and
> > the Pcd
> > redundant, and the rest of the code can use
> > MmioRead16()/MmioWrite16()
> > directly.
> > 
> > But then, it is not really a big-endian or litte-endian
> > version of the
> > library we need. We always know which endianness we are
> > building for.
> > What we need is a byteswapping flavour of IoLib.
> > 
> > So Liming, what if we do something like adding a
> > MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSwa
> > p.inf
> > ---
> > [Defines]
> >   INF_VERSION= 0x0001001A
> >   BASE_NAME  =
> > BaseIoLibIntrinsicSwap
> >   MODULE_UNI_FILE=
> > BaseIoLibIntrinsic.uni
> >   FILE_GUID  = d4a60d44-3688-4a50-
> > b2d0-5c6fc2422523
> >   MODULE_TYPE= BASE
> >   VERSION_STRING = 1.0
> >   LIBRARY_CLASS  = IoLib
> > 
> > 
> > #
> > #  VALID_ARCHITECTURES   = IA32 X64 EBC IPF ARM
> > AARCH64
> > #
> > 
> > [Sources]
> >   BaseIoLibIntrinsicInternal.h
> >   IoHighLevel.c
> >   IoLib.c
> >   IoLibEbc.c # Asserts on all i/o port accesses
> >   IoLibMmioBuffer.c
> > 
> > [Packages]
> >   MdePkg/MdePkg.dec
> > 
> > [LibraryClasses]
> >   DebugLib
> >   BaseLib
> > 
> > [BuildOptions]
> >   GCC:*_*_*_CC_FLAGS  = -DSWAP_BYTES
> > ---
> > 
> > And then add
> > 
> > #ifdef SWAP_BYTES
> >   return SwapBytesXX (Value);
> > #else
> >   return Value;
> > #fi
> > 
> > for the read operations and
> > 
> > #ifdef SWAP_BYTES
> >   *(type)Address = SwapBytesXX (Value);
> > #else
> >   *(type)Address = Value;
> > #fi
> > 
> > for the write operations in IoLib.c?
> > 
> > /
> > Leif
> > 
> > On Mon, Nov 27, 2017 at 06:06:33AM +, Meenakshi
> > Aggarwal wrote:
> > > Hi Leif,
> > >
> > > This is regarding Big-Endian Library patch ([PATCH v2
> > 1/9]
> > > Platform/NXP: Add support for Big Endian Mmio APIs)
> > >
> > > We have started this discussion before and the
> > suggestion was to
> > > create a separate .inf

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

2017-11-29 Thread Kinney, Michael D

Leif,

I agree that this should be described as byte swapping.

What about a module that needs to access HW with and
without the bytes swapped?  It gets more complex if a
module is linked against several libs and some libs
access HW with bytes swapped and some do not.

Thanks,

Mike

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-
> boun...@lists.01.org] On Behalf Of Leif Lindholm
> Sent: Wednesday, November 29, 2017 4:51 AM
> To: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>;
> Gao, Liming <liming@intel.com>
> Cc: Kinney, Michael D <michael.d.kin...@intel.com>;
> edk2-devel@lists.01.org; Ard Biesheuvel
> <ard.biesheu...@linaro.org>
> Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add
> support for big-endian MMIO
> 
> Hi Meenakshi,
> 
> I finally got around to looking at the watchdog code
> (that uses this
> library), and that has convinced me the best solution
> is to do what
> Liming proposed.
> 
> Looking at this snippet:
> 
> > +STATIC
> > +UINT16
> > +EFIAPI
> > +WdogRead (
> > +  IN  UINTN     Address
> > +  )
> > +{
> > +  if (FixedPcdGetBool (PcdWdogBigEndian)) {
> > +    return BeMmioRead16 (Address);
> > +  } else {
> > +    return MmioRead16(Address);
> > +  }
> 
> This is actually a pretty good demonstration of the
> arguments that
> were made with regards to how the BeIoLib could be just
> another
> implementation of IoLib.
> 
> You would then just do (in your .dsc):
>   Platform/NXP/Drivers/WatchDog/WatchDogDxe.inf {
>     IoLib|
>   }
> 
> This causes the big-endian version of the library to be
> used for this
> component. This makes these Wdog functions and
> the Pcd
> redundant, and the rest of the code can use
> MmioRead16()/MmioWrite16()
> directly.
> 
> But then, it is not really a big-endian or litte-endian
> version of the
> library we need. We always know which endianness we are
> building for.
> What we need is a byteswapping flavour of IoLib.
> 
> So Liming, what if we do something like adding a
> MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSwa
> p.inf
> ---
> [Defines]
>   INF_VERSION= 0x0001001A
>   BASE_NAME  =
> BaseIoLibIntrinsicSwap
>   MODULE_UNI_FILE=
> BaseIoLibIntrinsic.uni
>   FILE_GUID  = d4a60d44-3688-4a50-
> b2d0-5c6fc2422523
>   MODULE_TYPE= BASE
>   VERSION_STRING = 1.0
>   LIBRARY_CLASS  = IoLib
> 
> 
> #
> #  VALID_ARCHITECTURES   = IA32 X64 EBC IPF ARM
> AARCH64
> #
> 
> [Sources]
>   BaseIoLibIntrinsicInternal.h
>   IoHighLevel.c
>   IoLib.c
>   IoLibEbc.c # Asserts on all i/o port accesses
>   IoLibMmioBuffer.c
> 
> [Packages]
>   MdePkg/MdePkg.dec
> 
> [LibraryClasses]
>   DebugLib
>   BaseLib
> 
> [BuildOptions]
>   GCC:*_*_*_CC_FLAGS  = -DSWAP_BYTES
> ---
> 
> And then add
> 
> #ifdef SWAP_BYTES
>   return SwapBytesXX (Value);
> #else
>   return Value;
> #fi
> 
> for the read operations and
> 
> #ifdef SWAP_BYTES
>   *(type)Address = SwapBytesXX (Value);
> #else
>   *(type)Address = Value;
> #fi
> 
> for the write operations in IoLib.c?
> 
> /
> Leif
> 
> On Mon, Nov 27, 2017 at 06:06:33AM +, Meenakshi
> Aggarwal wrote:
> > Hi Leif,
> >
> > This is regarding Big-Endian Library patch ([PATCH v2
> 1/9]
> > Platform/NXP: Add support for Big Endian Mmio APIs)
> >
> > We have started this discussion before and the
> suggestion was to
> > create a separate .inf file keeping APIs name same
> > e.g. MmioRead/MmioWrite in MdePkg.
> >
> > But we can't go with this approach (reason mentioned
> by Udit).
> >
> > So please suggest if we should keep this library
> under Platform/NXP
> > or I send a new patch moving this library in MdePkg.
> >
> > But we have to keep a different name for Big Endian
> MMIO APIs.
> >
> >
> > Thanks,
> > Meenakshi
> >
> >
> > > -Original Message-
> > > From: Udit Kumar
> > > Sent: Monday, October 23, 2017 12:38 PM
> > > To: Gao, Liming <liming@intel.com>; Meenakshi
> Aggarwal
> > > <meenakshi.aggar...@nxp.com>; Ard Biesheuvel
> > > <ard.biesheu...@linaro.org>; Kinney, Michael D
> > > <michael.d.kin...@intel.com>; edk2-
> de...@lists.01.org
> > > Subject: RE: [edk2] [PATCH 1/1] MdePkg/IoLib: Add
> support for big-endian
> > > MMIO
> > >
> > > Hi Meenakshi/L

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

2017-11-29 Thread Leif Lindholm

Hi Meenakshi,

I finally got around to looking at the watchdog code (that uses this
library), and that has convinced me the best solution is to do what
Liming proposed.

Looking at this snippet:

> +STATIC
> +UINT16
> +EFIAPI
> +WdogRead (
> +  IN  UINTN     Address
> +  )
> +{
> +  if (FixedPcdGetBool (PcdWdogBigEndian)) {
> +    return BeMmioRead16 (Address);
> +  } else {
> +    return MmioRead16(Address);
> +  }

This is actually a pretty good demonstration of the arguments that
were made with regards to how the BeIoLib could be just another
implementation of IoLib.

You would then just do (in your .dsc):
  Platform/NXP/Drivers/WatchDog/WatchDogDxe.inf {
    IoLib|
  }

This causes the big-endian version of the library to be used for this
component. This makes these Wdog functions and the Pcd
redundant, and the rest of the code can use MmioRead16()/MmioWrite16()
directly.

But then, it is not really a big-endian or litte-endian version of the
library we need. We always know which endianness we are building for.
What we need is a byteswapping flavour of IoLib.

So Liming, what if we do something like adding a
MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSwap.inf
---
[Defines]
  INF_VERSION= 0x0001001A
  BASE_NAME  = BaseIoLibIntrinsicSwap
  MODULE_UNI_FILE= BaseIoLibIntrinsic.uni
  FILE_GUID  = d4a60d44-3688-4a50-b2d0-5c6fc2422523
  MODULE_TYPE= BASE
  VERSION_STRING = 1.0
  LIBRARY_CLASS  = IoLib


#
#  VALID_ARCHITECTURES   = IA32 X64 EBC IPF ARM AARCH64
#

[Sources]
  BaseIoLibIntrinsicInternal.h
  IoHighLevel.c
  IoLib.c
  IoLibEbc.c # Asserts on all i/o port accesses
  IoLibMmioBuffer.c

[Packages]
  MdePkg/MdePkg.dec

[LibraryClasses]
  DebugLib
  BaseLib

[BuildOptions]
  GCC:*_*_*_CC_FLAGS  = -DSWAP_BYTES
---

And then add

#ifdef SWAP_BYTES
  return SwapBytesXX (Value);
#else
  return Value;
#fi

for the read operations and

#ifdef SWAP_BYTES
  *(type)Address = SwapBytesXX (Value);
#else
  *(type)Address = Value;
#fi

for the write operations in IoLib.c?

/
Leif

On Mon, Nov 27, 2017 at 06:06:33AM +, Meenakshi Aggarwal wrote:
> Hi Leif,
> 
> This is regarding Big-Endian Library patch ([PATCH v2 1/9]
> Platform/NXP: Add support for Big Endian Mmio APIs)
> 
> We have started this discussion before and the suggestion was to
> create a separate .inf file keeping APIs name same
> e.g. MmioRead/MmioWrite in MdePkg.
> 
> But we can't go with this approach (reason mentioned by Udit).
> 
> So please suggest if we should keep this library under Platform/NXP
> or I send a new patch moving this library in MdePkg.
> 
> But we have to keep a different name for Big Endian MMIO APIs.
> 
> 
> Thanks,
> Meenakshi
> 
> 
> > -Original Message-
> > From: Udit Kumar
> > Sent: Monday, October 23, 2017 12:38 PM
> > To: Gao, Liming <liming@intel.com>; Meenakshi Aggarwal
> > <meenakshi.aggar...@nxp.com>; Ard Biesheuvel
> > <ard.biesheu...@linaro.org>; Kinney, Michael D
> > <michael.d.kin...@intel.com>; edk2-devel@lists.01.org
> > Subject: RE: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian
> > MMIO
> > 
> > Hi Meenakshi/Liming,
> > My 2 cents, around this.
> > 
> > 1)
> > Having a new lib for BE read might not be helpful for us, e.g. a IP which 
> > is in
> > BE mode access the UART for print or system registers which are in LE, then
> > with new Lib, we will get all read/write in BE mode
> > 
> > 2)
> > Especially for our IPs, which are changing from BE to LE depending on
> > platform.
> > As said before, having BE read lib with API name of MmioRead32 etc, will not
> > help (I guess Meenakshi already seen some problems around this) Adding a
> > new lib with MmioRead32BE API name could help, but IP driver we need to
> > take care of IP mode either by Pcd or #define, to select MmioRead32 or
> > MmioRead32BE.
> > This conditional compile needs to be done for all IPs (which works in BE/LE
> > mode on different platforms).
> > 
> > My preferred way of implementation to use one function in IP driver, And
> > based on IP mode, do the switch.
> > 
> > New Lib could have function like below
> > MmioRead32Generic(IN  UINTN Address, BOOL IsIPBE) {
> >UINT32Value;
> > 
> >ASSERT ((Address & 3) == 0);
> >Value = *(volatile UINT32*)Address;
> >If(IsIPBE)
> >  Value = SwapBytes32(Value);
> >  return  Value;
> > }
> > 
> > And IP driver can use it
> > MmioRead32Generic(ADDR,
> > FixedPcdGet(This_

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

2017-11-27 Thread Leif Lindholm

On Mon, Nov 27, 2017 at 06:06:33AM +, Meenakshi Aggarwal wrote:
> Hi Leif,
> 
> This is regarding Big-Endian Library patch ([PATCH v2 1/9]
> Platform/NXP: Add support for Big Endian Mmio APIs)
> 
> We have started this discussion before and the suggestion was to
> create a separate .inf file keeping APIs name same
> e.g. MmioRead/MmioWrite in MdePkg.
> 
> But we can't go with this approach (reason mentioned by Udit).
> 
> 
> So please suggest if we should keep this library under Platform/NXP
> or I send a new patch moving this library in MdePkg.

For now, absolutely keep it under Platform/NXP.

I do believe this will naturally make its way into edk2 at some point
in the future. Perhaps then as an alternative IoLib implementation to
override specific drivers (as suggested in the original thread).

> But we have to keep a different name for Big Endian MMIO APIs.

Sure.

Regards,

Leif
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

2017-11-26 Thread Meenakshi Aggarwal

Hi Leif,

This is regarding Big-Endian Library patch ([PATCH v2 1/9] Platform/NXP: Add 
support for Big Endian Mmio APIs)

We have started this discussion before and the suggestion was to create a 
separate .inf file keeping APIs name same e.g. MmioRead/MmioWrite in MdePkg.

But we can't go with this approach (reason mentioned by Udit).


So please suggest if we should keep this library under Platform/NXP or I send a 
new patch moving this library in MdePkg.

But we have to keep a different name for Big Endian MMIO APIs.


Thanks,
Meenakshi


> -Original Message-
> From: Udit Kumar
> Sent: Monday, October 23, 2017 12:38 PM
> To: Gao, Liming <liming@intel.com>; Meenakshi Aggarwal
> <meenakshi.aggar...@nxp.com>; Ard Biesheuvel
> <ard.biesheu...@linaro.org>; Kinney, Michael D
> <michael.d.kin...@intel.com>; edk2-devel@lists.01.org
> Subject: RE: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian
> MMIO
> 
> Hi Meenakshi/Liming,
> My 2 cents, around this.
> 
> 1)
> Having a new lib for BE read might not be helpful for us, e.g. a IP which is 
> in
> BE mode access the UART for print or system registers which are in LE, then
> with new Lib, we will get all read/write in BE mode
> 
> 2)
> Especially for our IPs, which are changing from BE to LE depending on
> platform.
> As said before, having BE read lib with API name of MmioRead32 etc, will not
> help (I guess Meenakshi already seen some problems around this) Adding a
> new lib with MmioRead32BE API name could help, but IP driver we need to
> take care of IP mode either by Pcd or #define, to select MmioRead32 or
> MmioRead32BE.
> This conditional compile needs to be done for all IPs (which works in BE/LE
> mode on different platforms).
> 
> My preferred way of implementation to use one function in IP driver, And
> based on IP mode, do the switch.
> 
> New Lib could have function like below
> MmioRead32Generic(IN  UINTN Address, BOOL IsIPBE) {
>UINT32Value;
> 
>ASSERT ((Address & 3) == 0);
>Value = *(volatile UINT32*)Address;
>If(IsIPBE)
>  Value = SwapBytes32(Value);
>  return  Value;
> }
> 
> And IP driver can use it
> MmioRead32Generic(ADDR,
> FixedPcdGet(This_IP_Mode_For_This_platform)
> 
> Comments are welcome.
> 
> Regards
> Udit
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > Gao, Liming
> > Sent: Monday, October 16, 2017 8:48 AM
> > To: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>; Ard Biesheuvel
> > <ard.biesheu...@linaro.org>; Kinney, Michael D
> > <michael.d.kin...@intel.com>; edk2-devel@lists.01.org
> > Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for
> > big-endian MMIO
> >
> > Meenakshi:
> >   I suggest to introduce new IoLib library instance, not to add new IoLib
> APIs.
> > New IoLib library instance will perform IO operation as the big
> > endian. You can update MdePkg/Library/BaseIoLibIntrinsic instance, add
> > new source file and new INF for it.
> >
> > UINT32
> > EFIAPI
> > MmioRead32 (
> >   IN  UINTN Address
> >   )
> > {
> >   UINT32Value;
> >
> >   ASSERT ((Address & 3) == 0);
> >   Value = *(volatile UINT32*)Address;
> >   return SwapBytes32(Value);
> > }
> >
> > Thanks
> > Liming
> > >-Original Message-
> > >From: Meenakshi Aggarwal [mailto:meenakshi.aggar...@nxp.com]
> > >Sent: Friday, October 13, 2017 2:07 PM
> > >To: Ard Biesheuvel <ard.biesheu...@linaro.org>; Kinney, Michael D
> > ><michael.d.kin...@intel.com>; edk2-devel@lists.01.org; Gao, Liming
> > ><liming@intel.com>
> > >Subject: RE: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for
> > >big-endian MMIO
> > >
> > >Hi All,
> > >
> > >
> > >It’s a pretty old discussion, we have left the upstreaming of NXP
> > >package in between because of some other work, but have started it
> > >again
> > now.
> > >
> > >
> > >Issue  : Few NXP modules support Big Endian MMIOs as these are ported
> > >from PowerPC.
> > >
> > >Solution suggested : Create a separate library for BE MMIO APIs.
> > >
> > >
> > >So what I have done is, I have created a separate library to support
> > >BE MMIO APIs and currently keeping it to my package.
> > >This library is basically a wrapper over existing MMIO APIs.
> > >
> > >UINT32
> > >EFIAPI

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

2017-10-23 Thread Udit Kumar

Hi Meenakshi/Liming, 
My 2 cents, around this. 

1)
Having a new lib for BE read might not be helpful for us, 
e.g. a IP which is in BE mode access the UART for print or system registers 
which are in LE, 
then with new Lib, we will get all read/write in BE mode 

2)
Especially for our IPs, which are changing from BE to LE depending on platform. 
As said before, having BE read lib with API name of MmioRead32 etc, will not 
help (I guess Meenakshi already seen some problems around this)
Adding a new lib with MmioRead32BE API name could help, but IP driver we need 
to take care of IP mode either by Pcd or #define, to select MmioRead32 or 
MmioRead32BE. 
This conditional compile needs to be done for all IPs (which works in BE/LE 
mode on different platforms). 

My preferred way of implementation to use one function in IP driver, 
And based on IP mode, do the switch. 

New Lib could have function like below 
MmioRead32Generic(IN  UINTN Address, BOOL IsIPBE) {
   UINT32Value;
 
   ASSERT ((Address & 3) == 0);
   Value = *(volatile UINT32*)Address;
   If(IsIPBE)
 Value = SwapBytes32(Value);
 return  Value;
}

And IP driver can use it 
MmioRead32Generic(ADDR, FixedPcdGet(This_IP_Mode_For_This_platform)

Comments are welcome. 

Regards
Udit

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Gao,
> Liming
> Sent: Monday, October 16, 2017 8:48 AM
> To: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>; Ard Biesheuvel
> <ard.biesheu...@linaro.org>; Kinney, Michael D <michael.d.kin...@intel.com>;
> edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian
> MMIO
> 
> Meenakshi:
>   I suggest to introduce new IoLib library instance, not to add new IoLib 
> APIs.
> New IoLib library instance will perform IO operation as the big endian. You 
> can
> update MdePkg/Library/BaseIoLibIntrinsic instance, add new source file and
> new INF for it.
> 
> UINT32
> EFIAPI
> MmioRead32 (
>   IN  UINTN Address
>   )
> {
>   UINT32Value;
> 
>   ASSERT ((Address & 3) == 0);
>   Value = *(volatile UINT32*)Address;
>   return SwapBytes32(Value);
> }
> 
> Thanks
> Liming
> >-Original Message-
> >From: Meenakshi Aggarwal [mailto:meenakshi.aggar...@nxp.com]
> >Sent: Friday, October 13, 2017 2:07 PM
> >To: Ard Biesheuvel <ard.biesheu...@linaro.org>; Kinney, Michael D
> ><michael.d.kin...@intel.com>; edk2-devel@lists.01.org; Gao, Liming
> ><liming@intel.com>
> >Subject: RE: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for
> >big-endian MMIO
> >
> >Hi All,
> >
> >
> >It’s a pretty old discussion, we have left the upstreaming of NXP
> >package in between because of some other work, but have started it again
> now.
> >
> >
> >Issue  : Few NXP modules support Big Endian MMIOs as these are ported
> >from PowerPC.
> >
> >Solution suggested : Create a separate library for BE MMIO APIs.
> >
> >
> >So what I have done is, I have created a separate library to support BE
> >MMIO APIs and currently keeping it to my package.
> >This library is basically a wrapper over existing MMIO APIs.
> >
> >UINT32
> >EFIAPI
> >BeMmioRead32 (
> >  IN  UINTN Address
> >  )
> >{
> >  UINT32  Value;
> >
> >  Value = MmioRead32(Address);
> >
> >  return SwapBytes32(Value);
> >}
> >
> >
> >Need your opinion on below optinos:
> >
> >1. Will this be a good idea to make this library a part of MdePkg? OR
> >
> >2. Add a new file e.g. IoBeMmio.c like IoHighLevel.c in
> >MdePkg/Library/BaseIoLibIntrinsic/
> > And made these APIs a part of IoLib itself. OR
> >
> >3. Keep this library internal to NXP package.
> >
> >
> >Please provide your inputs.
> >
> >
> >Thanks & Regards,
> >Meenakshi
> >
> >> -Original Message-
> >> From: Bhupesh Sharma
> >> Sent: Monday, October 17, 2016 3:28 PM
> >> To: Ard Biesheuvel <ard.biesheu...@linaro.org>; Kinney, Michael D
> >> <michael.d.kin...@intel.com>
> >> Cc: Gao, Liming <liming....@intel.com>; edk2-de...@ml01.01.org;
> >> Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>
> >> Subject: RE: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for
> >> big-endian MMIO
> >>
> >> Hi Ard,
> >>
> >> > -Original Message-
> >> > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> >> > Sent: Monday, October 17, 2016 1:12

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

2017-10-15 Thread Gao, Liming

Meenakshi:
  I suggest to introduce new IoLib library instance, not to add new IoLib APIs. 
New IoLib library instance will perform IO operation as the big endian. You can 
update MdePkg/Library/BaseIoLibIntrinsic instance, add new source file and new 
INF for it. 

UINT32
EFIAPI
MmioRead32 (
  IN  UINTN Address
  )
{
  UINT32Value;

  ASSERT ((Address & 3) == 0);
  Value = *(volatile UINT32*)Address;
  return SwapBytes32(Value);
}

Thanks
Liming
>-Original Message-
>From: Meenakshi Aggarwal [mailto:meenakshi.aggar...@nxp.com]
>Sent: Friday, October 13, 2017 2:07 PM
>To: Ard Biesheuvel <ard.biesheu...@linaro.org>; Kinney, Michael D
><michael.d.kin...@intel.com>; edk2-devel@lists.01.org; Gao, Liming
><liming....@intel.com>
>Subject: RE: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian
>MMIO
>
>Hi All,
>
>
>It’s a pretty old discussion, we have left the upstreaming of NXP package in
>between because of some other work, but have started it again now.
>
>
>Issue  : Few NXP modules support Big Endian MMIOs as these are ported from
>PowerPC.
>
>Solution suggested : Create a separate library for BE MMIO APIs.
>
>
>So what I have done is, I have created a separate library to support BE MMIO
>APIs and currently keeping it to my package.
>This library is basically a wrapper over existing MMIO APIs.
>
>UINT32
>EFIAPI
>BeMmioRead32 (
>  IN  UINTN Address
>  )
>{
>  UINT32  Value;
>
>  Value = MmioRead32(Address);
>
>  return SwapBytes32(Value);
>}
>
>
>Need your opinion on below optinos:
>
>1. Will this be a good idea to make this library a part of MdePkg? OR
>
>2. Add a new file e.g. IoBeMmio.c like IoHighLevel.c in
>MdePkg/Library/BaseIoLibIntrinsic/
> And made these APIs a part of IoLib itself. OR
>
>3. Keep this library internal to NXP package.
>
>
>Please provide your inputs.
>
>
>Thanks & Regards,
>Meenakshi
>
>> -Original Message-
>> From: Bhupesh Sharma
>> Sent: Monday, October 17, 2016 3:28 PM
>> To: Ard Biesheuvel <ard.biesheu...@linaro.org>; Kinney, Michael D
>> <michael.d.kin...@intel.com>
>> Cc: Gao, Liming <liming@intel.com>; edk2-de...@ml01.01.org;
>> Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>
>> Subject: RE: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian
>> MMIO
>>
>> Hi Ard,
>>
>> > -Original Message-----
>> > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>> > Sent: Monday, October 17, 2016 1:12 PM
>> > To: Kinney, Michael D <michael.d.kin...@intel.com>
>> > Cc: Gao, Liming <liming@intel.com>; Bhupesh Sharma
>> > <bhupesh.sha...@nxp.com>; edk2-de...@ml01.01.org
>> > Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-
>> > endian MMIO
>> >
>> > On 17 October 2016 at 05:10, Kinney, Michael D
>> > <michael.d.kin...@intel.com> wrote:
>> > > Bhupesh,
>> > >
>> > > It is also possible to add an ARM specific PCD to select endianness
>> > > and update MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c to use that
>> > > PCD in MmioRead/Write() APIs in that file to support both endian
>> > > types.  You can use the SwapBytesxx() functions from BaseLib(as
>> > Laszlo
>> > > suggested) based on the setting of this ARM specific PCD.
>> > >
>> > > Modules that link against this lib can select endianness by setting
>> > > PCD in the scope of that module.
>> > >
>> > > The IPF version of IoLib uses an IPF specific PCD to translate I/O
>> > > port accesses to MMIO accesses.  So there is already an example of
>> > > an arch specific PCD in this lib instance.
>> > >
>> >
>> > This is not a platform wide thing, it is a per-device property whether
>> > the MMIO occurs in big endian or little endian manner.
>> >
>> > So I think Liming's suggestion makes sense: create an IoLib
>> > implementation that performs the byte swapping, and selectively
>> > incorporate it into drivers that require it using
>> >
>> > BeMmioDeviceDxe.inf {
>> >   
>> > IoLib|SomePkg/Library/BigEndianIoLib.inf
>> > }
>>
>> That's correct. I think creating a separate IoLib for byte-swapping makes
>> sense.
>>
>> We will rework the patch accordingly.
>>
>> Regards,
>> Bhupesh
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

2017-10-13 Thread Meenakshi Aggarwal

Hi All,


It’s a pretty old discussion, we have left the upstreaming of NXP package in 
between because of some other work, but have started it again now.


Issue  : Few NXP modules support Big Endian MMIOs as these are ported from 
PowerPC.

Solution suggested : Create a separate library for BE MMIO APIs.


So what I have done is, I have created a separate library to support BE MMIO 
APIs and currently keeping it to my package.
This library is basically a wrapper over existing MMIO APIs.

UINT32
EFIAPI
BeMmioRead32 (
  IN  UINTN Address
  )
{
  UINT32  Value;

  Value = MmioRead32(Address);

  return SwapBytes32(Value);
}


Need your opinion on below optinos:

1. Will this be a good idea to make this library a part of MdePkg? OR

2. Add a new file e.g. IoBeMmio.c like IoHighLevel.c in 
MdePkg/Library/BaseIoLibIntrinsic/
 And made these APIs a part of IoLib itself. OR

3. Keep this library internal to NXP package.


Please provide your inputs.


Thanks & Regards,
Meenakshi

> -Original Message-
> From: Bhupesh Sharma
> Sent: Monday, October 17, 2016 3:28 PM
> To: Ard Biesheuvel <ard.biesheu...@linaro.org>; Kinney, Michael D
> <michael.d.kin...@intel.com>
> Cc: Gao, Liming <liming@intel.com>; edk2-de...@ml01.01.org;
> Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>
> Subject: RE: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian
> MMIO
> 
> Hi Ard,
> 
> > -Original Message-
> > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> > Sent: Monday, October 17, 2016 1:12 PM
> > To: Kinney, Michael D <michael.d.kin...@intel.com>
> > Cc: Gao, Liming <liming@intel.com>; Bhupesh Sharma
> > <bhupesh.sha...@nxp.com>; edk2-de...@ml01.01.org
> > Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-
> > endian MMIO
> >
> > On 17 October 2016 at 05:10, Kinney, Michael D
> > <michael.d.kin...@intel.com> wrote:
> > > Bhupesh,
> > >
> > > It is also possible to add an ARM specific PCD to select endianness
> > > and update MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c to use that
> > > PCD in MmioRead/Write() APIs in that file to support both endian
> > > types.  You can use the SwapBytesxx() functions from BaseLib(as
> > Laszlo
> > > suggested) based on the setting of this ARM specific PCD.
> > >
> > > Modules that link against this lib can select endianness by setting
> > > PCD in the scope of that module.
> > >
> > > The IPF version of IoLib uses an IPF specific PCD to translate I/O
> > > port accesses to MMIO accesses.  So there is already an example of
> > > an arch specific PCD in this lib instance.
> > >
> >
> > This is not a platform wide thing, it is a per-device property whether
> > the MMIO occurs in big endian or little endian manner.
> >
> > So I think Liming's suggestion makes sense: create an IoLib
> > implementation that performs the byte swapping, and selectively
> > incorporate it into drivers that require it using
> >
> > BeMmioDeviceDxe.inf {
> >   
> > IoLib|SomePkg/Library/BigEndianIoLib.inf
> > }
> 
> That's correct. I think creating a separate IoLib for byte-swapping makes
> sense.
> 
> We will rework the patch accordingly.
> 
> Regards,
> Bhupesh
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

2016-10-17 Thread Ard Biesheuvel

On 17 October 2016 at 05:10, Kinney, Michael D
 wrote:
> Bhupesh,
>
> It is also possible to add an ARM specific PCD to select endianness and update
> MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c to use that PCD in 
> MmioRead/Write()
> APIs in that file to support both endian types.  You can use the SwapBytesxx()
> functions from BaseLib(as Laszlo suggested) based on the setting of this ARM
> specific PCD.
>
> Modules that link against this lib can select endianness by setting PCD in the
> scope of that module.
>
> The IPF version of IoLib uses an IPF specific PCD to translate I/O port 
> accesses
> to MMIO accesses.  So there is already an example of an arch specific PCD in 
> this
> lib instance.
>

This is not a platform wide thing, it is a per-device property whether
the MMIO occurs in big endian or little endian manner.

So I think Liming's suggestion makes sense: create an IoLib
implementation that performs the byte swapping, and selectively
incorporate it into drivers that require it using

BeMmioDeviceDxe.inf {
  
IoLib|SomePkg/Library/BigEndianIoLib.inf
}
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

2016-10-16 Thread Kinney, Michael D

Bhupesh,

It is also possible to add an ARM specific PCD to select endianness and update
MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c to use that PCD in 
MmioRead/Write() 
APIs in that file to support both endian types.  You can use the SwapBytesxx()
functions from BaseLib(as Laszlo suggested) based on the setting of this ARM 
specific PCD.

Modules that link against this lib can select endianness by setting PCD in the 
scope of that module.

The IPF version of IoLib uses an IPF specific PCD to translate I/O port accesses
to MMIO accesses.  So there is already an example of an arch specific PCD in 
this
lib instance.

Mike

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Gao, 
> Liming
> Sent: Monday, October 17, 2016 11:10 AM
> To: Bhupesh Sharma <bhupesh.sha...@nxp.com>; edk2-de...@ml01.01.org
> Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO
> 
> Bhupesh:
>   In this patch, five class APIs are new added. They are
> MmioReadBe16(),MmioWriteBe16(),MmioClearSetBe16(),MmioSetBitsBe16(),MmioClearBitsBe16
> (). In fact, they can map to the existing MMIO APIs. Below is their mapping. 
> And, I
> understand some hardware uses little-endian MMIO interfaces, other hardware 
> use big-
> endian MMIO interfaces. But, there are no hardware to require little-endian 
> and big-
> endian at the same time. If so, we don't need to expose MmioRead16() and
> MmioReadBe16() API both in the same library instances. For your case, I 
> suggest to
> add new IoLib library instance that implement the existing MMIO APIs with the 
> big-
> endian way. This library instance can be placed into ARM device package.
> 
> MmioReadBe16()  --> MmioRead16()
> MmioWriteBe16() --> MmioWrite16()
> MmioClearSetBe16() -->  MmioAndThenOr16()
> MmioSetBitsBe16() --> MmioOr16()
> MmioClearBitsBe16() -->  MmioAnd16()
> 
> Thanks
> Liming
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > Bhupesh Sharma
> > Sent: Friday, October 14, 2016 5:34 PM
> > To: edk2-de...@ml01.01.org
> > Subject: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian
> > MMIO
> >
> > Various IPs on NXP/FSL SoCs having ARM64 cores have big-endian
> > MMIO interfaces.
> >
> > This implies that a byte-swap operation is needed to read/write
> > such BE MMIO registers from the LE ARM64 cores.
> >
> > This patch adds the support for the same.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>
> > Signed-off-by: Bhupesh Sharma <bhupesh.sha...@nxp.com>
> > ---
> >  MdePkg/Include/Library/IoLib.h   | 364 
> >  MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c | 479
> > +++
> >  2 files changed, 843 insertions(+)
> >
> > diff --git a/MdePkg/Include/Library/IoLib.h
> > b/MdePkg/Include/Library/IoLib.h
> > index a0dd16b..f5842e6 100644
> > --- a/MdePkg/Include/Library/IoLib.h
> > +++ b/MdePkg/Include/Library/IoLib.h
> > @@ -2658,6 +2658,370 @@ MmioWriteBuffer64 (
> >IN  CONST UINT64 *Buffer
> >);
> >
> > +/**
> > +  Reads a 16-bit MMIO register in Big Endian format.
> > +
> > +  Reads the 16-bit MMIO register specified by Address. The 16-bit read 
> > value
> > is
> > +  returned. This function must guarantee that all MMIO read and write
> > +  operations are serialized.
> > +
> > +  If 16-bit MMIO register operations are not supported, then ASSERT().
> > +
> > +  @param  Address The MMIO register to read.
> > +
> > +  @return The value read.
> > +
> > +**/
> > +UINT16
> > +EFIAPI
> > +MmioReadBe16 (
> > +  IN  UINTN Address
> > +  );
> > +
> > +/**
> > +  Writes a 16-bit MMIO register in Big Endian format.
> > +
> > +  Writes the 16-bit MMIO register specified by Address with the value
> > specified
> > +  by Value and returns Value. This function must guarantee that all MMIO
> > read
> > +  and write operations are serialized.
> > +
> > +  If 16-bit MMIO register operations are not supported, then ASSERT().
> > +
> > +  @param  Address The MMIO register to write.
> > +  @param  Value   The value to write to the MMIO register.
> > +
> > +**/
> > +UINT16
> > +EFIAPI
> > +MmioWriteBe16 (
> > +  IN  UINTN Address,
> > +  IN  UINT16

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

2016-10-16 Thread Gao, Liming

Bhupesh:
  In this patch, five class APIs are new added. They are 
MmioReadBe16(),MmioWriteBe16(),MmioClearSetBe16(),MmioSetBitsBe16(),MmioClearBitsBe16().
 In fact, they can map to the existing MMIO APIs. Below is their mapping. And, 
I understand some hardware uses little-endian MMIO interfaces, other hardware 
use big-endian MMIO interfaces. But, there are no hardware to require 
little-endian and big-endian at the same time. If so, we don't need to expose 
MmioRead16() and MmioReadBe16() API both in the same library instances. For 
your case, I suggest to add new IoLib library instance that implement the 
existing MMIO APIs with the big-endian way. This library instance can be placed 
into ARM device package. 

MmioReadBe16()  --> MmioRead16()
MmioWriteBe16() --> MmioWrite16()
MmioClearSetBe16() -->  MmioAndThenOr16()
MmioSetBitsBe16() --> MmioOr16()
MmioClearBitsBe16() -->  MmioAnd16()

Thanks
Liming
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Bhupesh Sharma
> Sent: Friday, October 14, 2016 5:34 PM
> To: edk2-de...@ml01.01.org
> Subject: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian
> MMIO
> 
> Various IPs on NXP/FSL SoCs having ARM64 cores have big-endian
> MMIO interfaces.
> 
> This implies that a byte-swap operation is needed to read/write
> such BE MMIO registers from the LE ARM64 cores.
> 
> This patch adds the support for the same.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>
> Signed-off-by: Bhupesh Sharma <bhupesh.sha...@nxp.com>
> ---
>  MdePkg/Include/Library/IoLib.h   | 364 
>  MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c | 479
> +++
>  2 files changed, 843 insertions(+)
> 
> diff --git a/MdePkg/Include/Library/IoLib.h
> b/MdePkg/Include/Library/IoLib.h
> index a0dd16b..f5842e6 100644
> --- a/MdePkg/Include/Library/IoLib.h
> +++ b/MdePkg/Include/Library/IoLib.h
> @@ -2658,6 +2658,370 @@ MmioWriteBuffer64 (
>IN  CONST UINT64 *Buffer
>);
> 
> +/**
> +  Reads a 16-bit MMIO register in Big Endian format.
> +
> +  Reads the 16-bit MMIO register specified by Address. The 16-bit read value
> is
> +  returned. This function must guarantee that all MMIO read and write
> +  operations are serialized.
> +
> +  If 16-bit MMIO register operations are not supported, then ASSERT().
> +
> +  @param  Address The MMIO register to read.
> +
> +  @return The value read.
> +
> +**/
> +UINT16
> +EFIAPI
> +MmioReadBe16 (
> +  IN  UINTN Address
> +  );
> +
> +/**
> +  Writes a 16-bit MMIO register in Big Endian format.
> +
> +  Writes the 16-bit MMIO register specified by Address with the value
> specified
> +  by Value and returns Value. This function must guarantee that all MMIO
> read
> +  and write operations are serialized.
> +
> +  If 16-bit MMIO register operations are not supported, then ASSERT().
> +
> +  @param  Address The MMIO register to write.
> +  @param  Value   The value to write to the MMIO register.
> +
> +**/
> +UINT16
> +EFIAPI
> +MmioWriteBe16 (
> +  IN  UINTN Address,
> +  IN  UINT16Value
> +  );
> +
> +/**
> +  Reads a 32-bit MMIO register in Big Endian format.
> +
> +  Reads the 32-bit MMIO register specified by Address. The 32-bit read value
> is
> +  returned. This function must guarantee that all MMIO read and write
> +  operations are serialized.
> +
> +  If 32-bit MMIO register operations are not supported, then ASSERT().
> +
> +  @param  Address The MMIO register to read.
> +
> +  @return The value read.
> +
> +**/
> +UINT32
> +EFIAPI
> +MmioReadBe32 (
> +  IN  UINTN Address
> +  );
> +
> +/**
> +  Writes a 32-bit MMIO register in Big Endian format.
> +
> +  Writes the 32-bit MMIO register specified by Address with the value
> specified
> +  by Value and returns Value. This function must guarantee that all MMIO
> read
> +  and write operations are serialized.
> +
> +  If 32-bit MMIO register operations are not supported, then ASSERT().
> +
> +  @param  Address The MMIO register to write.
> +  @param  Value   The value to write to the MMIO register.
> +
> +**/
> +UINT32
> +EFIAPI
> +MmioWriteBe32 (
> +  IN  UINTN Address,
> +  IN  UINT32Value
> +  );
> +
> +
> +/**
> +  Reads a 64-bit MMIO register in Big Endian format.
> +
> +  Reads the 64-bit MMIO register specified by Address. The 64-bit read value
> is
> +  returned

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

2016-10-14 Thread Laszlo Ersek

On 10/14/16 15:17, Bhupesh Sharma wrote:
> Hi Laszlo,
> 
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Friday, October 14, 2016 5:37 PM
>>
>> On 10/14/16 11:33, Bhupesh Sharma wrote:
>>> Various IPs on NXP/FSL SoCs having ARM64 cores have big-endian MMIO
>>> interfaces.
>>>
>>> This implies that a byte-swap operation is needed to read/write such
>>> BE MMIO registers from the LE ARM64 cores.
>>>
>>> This patch adds the support for the same.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Meenakshi Aggarwal 
>>> Signed-off-by: Bhupesh Sharma 
>>> ---
>>>  MdePkg/Include/Library/IoLib.h   | 364
>> 
>>>  MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c | 479
>>> +++
>>>  2 files changed, 843 insertions(+)
>>
>> I think this is both overkill and incomplete, at the same time :)
>>
>> - Incomplete because only one IoLib instance gets the implementation.
> 
> Agree, but do we have an example of similar BE MMIO IPs on other
> Architectures/soc? I was only aware of NXP/FSL having such MMIO
> interfaces as the IPs have been reused from PPC SoC, which used
> to have a BE core and hence did not require a SwapByte.

For example, the fw_cfg device of QEMU is big endian. OvmfPkg and
ArmVirtPkg call SwapBytesXX as necessary, in combination with MmioWriteXX.

In general, I think it's expected that a library declared under MdePkg
will not cause build failures (unresolved symbols) in any platform code,
once the library is resolved correctly in the platform DSC.

>  
>> - Overkill because you can easily use the SwapBytes16, SwapBytes32,
>> SwapBytes64 functions -- also from BaseLib --, for transforming
>> MmioWrite arguments and MmioRead results.
>>
> 
> Yes, but that means at every IP driver needs to especially carry such
> arguments and transform the results.

Indeed.

> That might be an overkill.

I think it should be possible to factor out these functions to a
separate library, or non-standard protocol, that is central enough for
the platform or device in question, but not core enough for putting into
MdePkg.

> We already have similar implementations MMIO implementations in Linux for e.g.
> http://lxr.free-electrons.com/source/include/asm-generic/io.h#L642

Hmmm. That does detract from the value of my "overkill" argument. So I
guess I'll have to defer to the MdePkg maintainers on that.

Regarding my "incomplete" argument, it still stands. I think it's
logically impossible to introduce a library class that is simultaneously
- central enough to merit a place under MdePkg, but
- not central enough to receive implementations (library instances) for
all the platforms supported by edk2 at the moment.

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

2016-10-14 Thread Bhupesh Sharma

Hi Laszlo,

> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Friday, October 14, 2016 5:37 PM
> 
> On 10/14/16 11:33, Bhupesh Sharma wrote:
> > Various IPs on NXP/FSL SoCs having ARM64 cores have big-endian MMIO
> > interfaces.
> >
> > This implies that a byte-swap operation is needed to read/write such
> > BE MMIO registers from the LE ARM64 cores.
> >
> > This patch adds the support for the same.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Meenakshi Aggarwal 
> > Signed-off-by: Bhupesh Sharma 
> > ---
> >  MdePkg/Include/Library/IoLib.h   | 364
> 
> >  MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c | 479
> > +++
> >  2 files changed, 843 insertions(+)
> 
> I think this is both overkill and incomplete, at the same time :)
> 
> - Incomplete because only one IoLib instance gets the implementation.

Agree, but do we have an example of similar BE MMIO IPs on other
Architectures/soc? I was only aware of NXP/FSL having such MMIO
interfaces as the IPs have been reused from PPC SoC, which used
to have a BE core and hence did not require a SwapByte.

> - Overkill because you can easily use the SwapBytes16, SwapBytes32,
> SwapBytes64 functions -- also from BaseLib --, for transforming
> MmioWrite arguments and MmioRead results.
> 

Yes, but that means at every IP driver needs to especially carry such
arguments and transform the results. That might be an overkill.

We already have similar implementations MMIO implementations in Linux for e.g.
http://lxr.free-electrons.com/source/include/asm-generic/io.h#L642

Regards,
Bhupesh
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

2016-10-14 Thread Laszlo Ersek

On 10/14/16 11:33, Bhupesh Sharma wrote:
> Various IPs on NXP/FSL SoCs having ARM64 cores have big-endian
> MMIO interfaces.
> 
> This implies that a byte-swap operation is needed to read/write
> such BE MMIO registers from the LE ARM64 cores.
> 
> This patch adds the support for the same.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Meenakshi Aggarwal 
> Signed-off-by: Bhupesh Sharma 
> ---
>  MdePkg/Include/Library/IoLib.h   | 364 
>  MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c | 479 
> +++
>  2 files changed, 843 insertions(+)

I think this is both overkill and incomplete, at the same time :)

- Incomplete because only one IoLib instance gets the implementation.

- Overkill because you can easily use the SwapBytes16, SwapBytes32,
SwapBytes64 functions -- also from BaseLib --, for transforming
MmioWrite arguments and MmioRead results.

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

2016-10-14 Thread Bhupesh Sharma

Various IPs on NXP/FSL SoCs having ARM64 cores have big-endian
MMIO interfaces.

This implies that a byte-swap operation is needed to read/write
such BE MMIO registers from the LE ARM64 cores.

This patch adds the support for the same.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Meenakshi Aggarwal 
Signed-off-by: Bhupesh Sharma 
---
 MdePkg/Include/Library/IoLib.h   | 364 
 MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c | 479 +++
 2 files changed, 843 insertions(+)

diff --git a/MdePkg/Include/Library/IoLib.h b/MdePkg/Include/Library/IoLib.h
index a0dd16b..f5842e6 100644
--- a/MdePkg/Include/Library/IoLib.h
+++ b/MdePkg/Include/Library/IoLib.h
@@ -2658,6 +2658,370 @@ MmioWriteBuffer64 (
   IN  CONST UINT64 *Buffer
   );
 
+/**
+  Reads a 16-bit MMIO register in Big Endian format.
+
+  Reads the 16-bit MMIO register specified by Address. The 16-bit read value is
+  returned. This function must guarantee that all MMIO read and write
+  operations are serialized.
+
+  If 16-bit MMIO register operations are not supported, then ASSERT().
+
+  @param  Address The MMIO register to read.
+
+  @return The value read.
+
+**/
+UINT16
+EFIAPI
+MmioReadBe16 (
+  IN  UINTN Address
+  );
+
+/**
+  Writes a 16-bit MMIO register in Big Endian format.
+
+  Writes the 16-bit MMIO register specified by Address with the value specified
+  by Value and returns Value. This function must guarantee that all MMIO read
+  and write operations are serialized.
+
+  If 16-bit MMIO register operations are not supported, then ASSERT().
+
+  @param  Address The MMIO register to write.
+  @param  Value   The value to write to the MMIO register.
+
+**/
+UINT16
+EFIAPI
+MmioWriteBe16 (
+  IN  UINTN Address,
+  IN  UINT16Value
+  );
+
+/**
+  Reads a 32-bit MMIO register in Big Endian format.
+
+  Reads the 32-bit MMIO register specified by Address. The 32-bit read value is
+  returned. This function must guarantee that all MMIO read and write
+  operations are serialized.
+
+  If 32-bit MMIO register operations are not supported, then ASSERT().
+
+  @param  Address The MMIO register to read.
+
+  @return The value read.
+
+**/
+UINT32
+EFIAPI
+MmioReadBe32 (
+  IN  UINTN Address
+  );
+
+/**
+  Writes a 32-bit MMIO register in Big Endian format.
+
+  Writes the 32-bit MMIO register specified by Address with the value specified
+  by Value and returns Value. This function must guarantee that all MMIO read
+  and write operations are serialized.
+
+  If 32-bit MMIO register operations are not supported, then ASSERT().
+
+  @param  Address The MMIO register to write.
+  @param  Value   The value to write to the MMIO register.
+
+**/
+UINT32
+EFIAPI
+MmioWriteBe32 (
+  IN  UINTN Address,
+  IN  UINT32Value
+  );
+
+
+/**
+  Reads a 64-bit MMIO register in Big Endian format.
+
+  Reads the 64-bit MMIO register specified by Address. The 64-bit read value is
+  returned. This function must guarantee that all MMIO read and write
+  operations are serialized.
+
+  If 64-bit MMIO register operations are not supported, then ASSERT().
+
+  @param  Address The MMIO register to read.
+
+  @return The value read.
+
+**/
+UINT64
+EFIAPI
+MmioReadBe64 (
+  IN  UINTN Address
+  );
+
+
+/**
+  Writes a 64-bit MMIO register in Big Endian format.
+
+  Writes the 64-bit MMIO register specified by Address with the value specified
+  by Value and returns Value. This function must guarantee that all MMIO read
+  and write operations are serialized.
+
+  If 64-bit MMIO register operations are not supported, then ASSERT().
+
+  @param  Address The MMIO register to write.
+  @param  Value   The value to write to the MMIO register.
+
+**/
+UINT64
+EFIAPI
+MmioWriteBe64 (
+  IN  UINTN Address,
+  IN  UINT64Value
+  );
+
+/**
+  Clear and set a 8-bit MMIO register.
+
+  Mask the 8-bit MMIO register specified by Address with the mask specified
+  by Mask and then Writes the 8-bit MMIO register specified by Address with
+  the value specified by Value and returns current value on register. This
+  function must guarantee that all MMIO read and write operations are 
serialized.
+
+  @param  Address The MMIO register to write.
+  @param  MaskThe Mask to clear the MMIO register.
+  @param  Value   The value to write to the MMIO register.
+
+**/
+UINT8
+EFIAPI
+MmioClearSet8 (
+  IN  UINTNAddress,
+  IN  UINT8Mask,
+  IN  UINT8Value
+  );
+
+/**
+  Clear and set a 16-bit MMIO register in Big Endian format.
+
+  Mask the 16-bit MMIO register specified by Address with the mask specified
+  by Mask and then Writes the 16-bit MMIO register specified by Address

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

[edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

22 matches

Site Navigation

Mail list logo

Footer information