[U-Boot] [PATCH 2/2] mpc85xx: Add support for the Varisys Cyrus board

[York Sun]

On 11/02/2015 10:56 PM, Andy Fleming wrote:
> On Fri, Oct 30, 2015 at 12:10 PM, York Sun <yorksun at freescale.com> wrote:
>>
>>
>> On 10/21/2015 04:59 PM, Andy Fleming wrote:
>>> This board runs a P5020 or P5040 chip, and utilizes
>>> an EEPROM with similar formatting to the Freescale P5020DS.
>>>
>>> Large amounts of this code were developed by
>>> Adrian Cox <adrian at humboldt dot co dot uk>
>>>
>>> Signed-off-by: Andy Fleming <afleming at gmail.com>
>>> ---
>>>  arch/powerpc/cpu/mpc85xx/Kconfig     |   4 +
>>>  board/varisys/common/Makefile        |  23 ++
>>>  board/varisys/common/eeprom.h        |   6 +
>>>  board/varisys/common/sys_eeprom.c    | 605 +++++++++++++++++++++++++++++++++++
>>>  board/varisys/cyrus/Kconfig          |  13 +
>>>  board/varisys/cyrus/Makefile         |   8 +
>>>  board/varisys/cyrus/README           |  21 ++
>>>  board/varisys/cyrus/cyrus.c          | 116 +++++++
>>>  board/varisys/cyrus/cyrus.h          |  11 +
>>>  board/varisys/cyrus/ddr.c            | 188 +++++++++++
>>>  board/varisys/cyrus/eth.c            | 100 ++++++
>>>  board/varisys/cyrus/law.c            |  27 ++
>>>  board/varisys/cyrus/pbi.cfg          |  35 ++
>>>  board/varisys/cyrus/pci.c            |  23 ++
>>>  board/varisys/cyrus/rcw_p5020_v2.cfg |  11 +
>>>  board/varisys/cyrus/rcw_p5040.cfg    |  11 +
>>>  board/varisys/cyrus/tlb.c            | 106 ++++++
>>>  configs/Cyrus_P5020_defconfig        |   9 +
>>>  configs/Cyrus_P5040_defconfig        |   9 +
>>>  include/configs/cyrus.h              | 590 ++++++++++++++++++++++++++++++++++
>>>  20 files changed, 1916 insertions(+)
>>>  create mode 100644 board/varisys/common/Makefile
>>>  create mode 100644 board/varisys/common/eeprom.h
>>>  create mode 100644 board/varisys/common/sys_eeprom.c
>>>  create mode 100644 board/varisys/cyrus/Kconfig
>>>  create mode 100644 board/varisys/cyrus/Makefile
>>>  create mode 100644 board/varisys/cyrus/README
>>>  create mode 100644 board/varisys/cyrus/cyrus.c
>>>  create mode 100644 board/varisys/cyrus/cyrus.h
>>>  create mode 100644 board/varisys/cyrus/ddr.c
>>>  create mode 100644 board/varisys/cyrus/eth.c
>>>  create mode 100644 board/varisys/cyrus/law.c
>>>  create mode 100644 board/varisys/cyrus/pbi.cfg
>>>  create mode 100644 board/varisys/cyrus/pci.c
>>>  create mode 100644 board/varisys/cyrus/rcw_p5020_v2.cfg
>>>  create mode 100644 board/varisys/cyrus/rcw_p5040.cfg
>>>  create mode 100644 board/varisys/cyrus/tlb.c
>>>  create mode 100644 configs/Cyrus_P5020_defconfig
>>>  create mode 100644 configs/Cyrus_P5040_defconfig
>>>  create mode 100644 include/configs/cyrus.h
>>>
>>
>> Andy,
>>
>> I presume you have examined the difference between
>> board/varisys/common/sys_eeprom.c and the original file
>> board/freescale/common/sys_eeprom.c. Is it possible to reuse the existing file?
>> This is a 600+ lines copy-n-paste.
> 
> This code presents some challenges. The code is nearly identical to
> the Freescale code, and quite unnecessarily so. If I were working at
> Varisys, I would recommend that we pare down this code to only what we
> need. As I am not, and as I presume that some amount of eeprom
> programming may be done in manufacturing, I don't feel comfortable
> changing this code (other than to perhaps remove the MPC85xx-specific
> stuff I missed).
> 
> Sadly, while it is nearly identical to the Freescale code, there are a
> number of differences. This code uses NXID version 0, which has been
> removed from the FSL code. It also supports an alternate mechanism for
> reading the eeprom (mac_read_from_generic_eeprom).
> 
> I can imagine some ways I might refactor the code to ensure that it
> works for both FSL and Varisys systems, but this raises the issue that
> these files implement two completely separate APIs which are only
> nearly-identical in current implementation. Freescale could decide to
> change the format of their eeproms, and provide scripts to any
> affected customers to shift them to the new format (they have before).
> Freescale could discover that manufacturing made a mistake in the
> formatting, and submit code to accommodate this. Likewise, Varisys
> could decide that the current format is more than they need, or that
> it's redundant with some other board mechanism. In such cases,
> unifying this code would actually make code maintenance *more*
> difficult for the two companies.
> 
> I did initially intend to refactor the code, but changed my mind when
> I realized how difficult it would be to make sure I hadn't broken
> anyone else's board. If you think it's necessary, I would probably
> just try to strip down the varisys version of the file to the bare
> essentials. I'm just not sure which parts *are* essential at the
> moment.

Andy,

It makes sense to keep code separated for different manufactures. Some comments
in the file header to explain what you just said will be beneficial for
maintenance. And if you can strip down unneeded code, that will reduce the size
and clearly make it different from the existing code used by Freescale boards.

York