[coreboot-gerrit] Change in coreboot[master]: soc/intel/common/block: Add Intel common UART code

[Aamir Bohra (Code Review)]

Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/18952 )

Change subject: soc/intel/common/block: Add Intel common UART code
......................................................................


Patch Set 1:

(10 comments)

https://review.coreboot.org/#/c/18952/1//COMMIT_MSG
Commit Message:

PS1, Line 9: .This
> Please add a space after the period/dot.
Done


https://review.coreboot.org/#/c/18952/1/src/soc/intel/common/block/include/intelblocks/uart.h
File src/soc/intel/common/block/include/intelblocks/uart.h:

PS1, Line 20: #define SIO_REG_PPR_CLOCK	0x200
            : #define SIO_REG_PPR_CLOCK_EN	(1 << 0)
            : #define SIO_REG_PPR_CLOCK_UPDATE	(1 << 31)
            : 
            : #define SIO_REG_PPR_RESETS	0x204
            : #define SIO_REG_PPR_RESETS_FUNC	(1 << 0)
            : #define SIO_REG_PPR_RESETS_APB	(1 << 1)
> Why are these macros exposed to the user?
These are to be used in UART common driver code.Sorry,I did not understand on the comment.Do you suggest to remove it and use it in offsets and config values directly.


https://review.coreboot.org/#/c/18952/1/src/soc/intel/common/block/uart/Makefile.inc
File src/soc/intel/common/block/uart/Makefile.inc:

Line 2: 
> Please remove the blank line at end of line. (At least Gerrit displays it l
Done


https://review.coreboot.org/#/c/18952/1/src/soc/intel/common/block/uart/uart.c
File src/soc/intel/common/block/uart/uart.c:

PS1, Line 24: PCH_DEVFN_UART2;
> Lets use this 
Done


PS1, Line 24: PCH_DEVFN_UART2;
> Why is this function assuming the UART to use?
Ok.Revised in Patchset 2,device is now passed as argument.


PS1, Line 29: (uint32_t)base
> Use the baseaddr variable. It's already there w/o casting pointers to uint3
Ok.Removed.


PS1, Line 38: SIO_REG_PPR_RESETS_FUNC | SIO_REG_PPR_RESETS_APB
> What SoCs does this apply to?
This applies to Skylake/Apollolake/GLK. The expected reset programming is to set Bit0 and Bit 1(uart reset field).However current skylake implementation is setting single register field as bit wise.
Revised implementation to set register field configuration as one field rather than bit wise.


PS1, Line 41: /*
            : 	 * Set M and N divisor inputs and enable clock.
            : 	 */
> Do it in one line?
Done


Line 47: 		(SIO_REG_PPR_CLOCK_M_DIV << 1);
> What SoCs do these magic constants apply?
This are Bit offsets for M_VAL and N_VAL of Clock register (0x200),which decides clock divider parameter.These offsets are common to Skylake/Apollolake/GLK.


Line 49: 
> Pleas remove the empty line.
Done


-- 
To view, visit https://review.coreboot.org/18952
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3843fac88cfb7bbb405be50d69f555b274f0d72a
Gerrit-PatchSet: 1
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Aamir Bohra <aamir.bohra at intel.com>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra at intel.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Barnali Sarkar <barnali.sarkar at intel.com>
Gerrit-Reviewer: Hannah Williams <hannah.williams at intel.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Subrata Banik <subrata.banik at intel.com>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-HasComments: Yes