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

["Leif Lindholm" <leif@nuviainc.com>]

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