Re: [PATCH edk2-platforms 1/5] Silicon/NXP/LS1043A: Fix the Platform PLL calculation

["Pankaj Bansal" <pankaj.bansal@nxp.com>]

Hi Leif,

Sorry for late reply.
Actually I had asked the design team to check this point.
I am waiting for their reply.
I will add comments in code based on their reply and send new version.

Regards,
Pankaj Bansal

> -----Original Message-----
> From: Leif Lindholm <leif@nuviainc.com>
> Sent: Friday, June 12, 2020 8:41 PM
> To: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>
> Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Michael D Kinney
> <michael.d.kinney@intel.com>; devel@edk2.groups.io; Varun Sethi
> <V.Sethi@nxp.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> Mahmoud@arm.com>; Augustine Philips <Augustine.Philips@arm.com>; Ard
> Biesheuvel <ard.biesheuvel@linaro.org>; Arokia Samy
> <arokia.samy@puresoftware.com>; kuldip dwivedi
> <kuldip.dwivedi@puresoftware.com>
> Subject: Re: [PATCH edk2-platforms 1/5] Silicon/NXP/LS1043A: Fix the Platform
> PLL calculation
> 
> Hi Pankaj,
> 
> Apologies for delay in responding, this message got lost from my inbox.
> On a sidenote, I think this has something to do with the email
> moderation. Could you possibly subscribe the @oss.nxp.com address to
> the list? You can set it not to deliver email, under
> https://edk2.groups.io/g/devel/editsub, but I can't whitelist
> addresses that are not subscribed.
> 
> On Mon, Jun 08, 2020 at 19:56:35 +0000, Pankaj Bansal (OSS) wrote:
> > > > > OK, now I'm confused.
> > > > > DCFG is read using the DcfgRead32 function, which is supposed to
> > > > > handle the endianness issue.
> > > > >
> > > > > Ls1043a builds with
> > > > >   gNxpQoriqLsTokenSpaceGuid.PcdDcfgBigEndian|TRUE
> > > > > which means GetMmioOperations() returns the byte-swapping versions.
> > > > >
> > > > > Please clarify.
> > > >
> > > > OK. so this might be little confusing, so bear with me.
> > > > The reset configuration word (RCW) is 512 bits (1024 bits in LS2088
> > > > / LS2160) long and contains all necessary configuration information
> > > > for the chip. RCW data is read from external memory (Nor flash or
> > > > SD/eMMC card or I2c eeprom) and written to the RCW status registers
> > > > (RCWSR) contained in the Device Configuration and Pin Control module
> > > > (DCSR), after which the device is configured as specified in the
> > > > RCW.
> > > >
> > > > The PreBoot Loader (PBL) fetches RCW data from the source memory
> > > > device and writes it to the RCW status registers.
> > > > Now the PBL fetches the data from flash in little endian format and
> > > > writes it to the DCSR registers in little endian format always.
> > > > This steps is same for all SOCs (LX2160 / LS1043 / LS1046 / LS2088).
> > >
> > > This PBL is a ROM executing before the EDK2 code?
> >
> > Yes
> >
> > >
> > > > Now in SOCs where DCSR space is big endian (LS1043 / LS1046), we
> > > > read the RCWSR registers in big endian fashion.
> > > > This causes the bit position to be reversed.
> > >
> > > I'm still not following.
> > >
> > > We've set up this elaborate Rube Goldberg machine to be able to *not*
> > > have to carry separate header files for devices with individual
> > > components with registers that may be big- or little-endian depending
> > > on which SoC/version they are in.
> > >
> > > And now we have an implementation that states that its DcfgRead
> > > operations need to happan as big-endian. And the *only* time the Dcfg
> > > registers are accessed, we immediately need to change the header file
> > > to treat it as little-endian?
> >
> > The RCW Status registers are a special case and a subset of DCFG
> > address space. The whole DCFG address space is big endian itself,
> > and should be read as such.
> 
> So the RCW status registers are in effect just temporary storage for
> data, as opposed to having any effect on the hw? Whereas other parts
> of DCFG *do* affect (and reflect) hw, and are big-endian?
> 
> If so, ok, I understand. And I think your platform designers owe me
> (and you, if so inclined) a beer.
> 
> > if it makes more sense, then I can swap the RCW status registers
> > after being read from DCFG space.
> > And I can put the explanation I wrote above in the code where I swap
> > RCW SR registers ?
> 
> Yes, I think manually swapping the words make more sense. This is a
> *weird* thing - it helps to call it out explicitly rather than try to
> make it look normal.
> 
> Please do that, and drop the .h change, and I'm happy with that.
> 
> > > What is the situation where Dcfg accesses *need* to be big-endian?
> >
> > Apart from RCWSR registers the DCFG space contains following
> > registers as well, which we need to access in boot firmware:
> >
> > - SVR (SOC Version Register)
> > - to retrieve Core and Cluster Information (which I plan to send shortly)
> > - To set the ICID of DMA connected devices like USB, SATA, SD/EMMC
> > - to retrieve the clock frequency of serial flash controller (qspi/flexspi)
> 
> Understood - thanks!
> 
> /
>     Leif