[coreboot-gerrit] Change in coreboot[master]: soc/intel/common/block:[WIP] Create header for GPIO controll...

[Hannah Williams (Code Review)]

Hannah Williams has posted comments on this change. ( https://review.coreboot.org/18892 )

Change subject: soc/intel/common/block:[WIP] Create header for GPIO controller config
......................................................................


Patch Set 8:

(6 comments)

https://review.coreboot.org/#/c/18892/8/src/soc/intel/common/block/include/intelblocks/gpio.h
File src/soc/intel/common/block/include/intelblocks/gpio.h:

Line 21: #include <types.h>
> In looking through existing released SoCs and forthcoming ones, what is dif
What I found was that newer ones were only trying to use the reserved bits from before. Hence backwards compatibility is not an issue. We can have PAD_CONFIG0_MASK and PAD_CONFIG1_MASK in soc code to leave out the reserved bits which would be different for different SOCs. However I do not know about forthcoming ones. I only compared EDS of SKL, APL and GLK


Line 50:   GPIO_D = 1, // GPIO driver
> Not everything in this file is tab indented.
will fix


Line 86: #define M(mode)			(mode)
> What is this for? Needing a define from soc/gpio.h?
the number of bits used for mode is different in different SOCs


Line 250: #define GLK_GPIO_PAD1_CONF(ios_state, term_H_L, ios_term)		\
> So this is gemini lake only?
I am missing INTSEL - on adding that it should be common and not just GLK


Line 266:         const char		*pad_name;
> This is a huge structure to maintain per pad config. 24 bytes per pad?
I can put  pad_name in only for debug coreboot


Line 281: typedef void(*config_write)(int pad,  int reg_offset, uint32_t value);
> Why the need for typedefs?
I was going to remove the direct access to   iosf_read\write and let the SOC layer decide the read\write access function. Not that it is anything other than iosf read\write today but what if a different access type is needed in the future


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I09cc3df4749288d3772600915363549d731cebab
Gerrit-PatchSet: 8
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Hannah Williams <hannah.williams 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-Reviewer: dhaval v sharma <dhaval.v.sharma at intel.com>
Gerrit-HasComments: Yes