lkml.org 
[lkml]   [2018]   [Jan]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH arm/aspeed/ast2500 v2] ipmi: add an Aspeed KCS IPMI BMC driver
From
Date


On 2018-01-25 01:05, Andy Shevchenko wrote:
> On Thu, 2018-01-25 at 00:06 +0800, Haiyue Wang wrote:
>> The KCS (Keyboard Controller Style) interface is used to perform in-
>> band
>> IPMI communication between a server host and its BMC (BaseBoard
>> Management
>> Controllers).
>>
>>
>> +config ASPEED_KCS_IPMI_BMC
>> + depends on ARCH_ASPEED || COMPILE_TEST
>> + depends on IPMI_KCS_BMC
>> + select REGMAP_MMIO
>> + tristate "Aspeed KCS IPMI BMC driver"
>> + help
>> + Provides a driver for the KCS (Keyboard Controller Style)
>> IPMI
>> + interface found on Aspeed SOCs (AST2400 and AST2500).
>> +
>> + The driver implements the BMC side of the KCS contorller,
>> it
>> + provides the access of KCS IO space for BMC side.
>> +static inline u8 read_data(struct kcs_bmc *kcs_bmc)
>> +{
>> + return kcs_bmc->io_inputb(kcs_bmc, kcs_bmc->ioreg.idr);
>> +}
>> +
>> +static inline void write_data(struct kcs_bmc *kcs_bmc, u8 data)
>> +{
>> + kcs_bmc->io_outputb(kcs_bmc, kcs_bmc->ioreg.odr, data);
>> +}
>> +
>> +static inline u8 read_status(struct kcs_bmc *kcs_bmc)
>> +{
>> + return kcs_bmc->io_inputb(kcs_bmc, kcs_bmc->ioreg.str);
>> +}
>> +
>> +static inline void write_status(struct kcs_bmc *kcs_bmc, u8 data)
>> +{
>> + kcs_bmc->io_outputb(kcs_bmc, kcs_bmc->ioreg.str, data);
>> +}
>> +
>> +static void update_status_bits(struct kcs_bmc *kcs_bmc, u8 mask, u8
>> val)
>> +{
>> + u8 tmp;
>> +
>> + tmp = read_status(kcs_bmc);
>> +
>> + tmp &= ~mask;
>> + tmp |= val & mask;
>> +
>> + write_status(kcs_bmc, tmp);
>> +}
> Shouldn't be above some kind of regmap API?
It is KCS spec defined IO access for hidden the low level, if the low
level supports regmap, such as in kcs_bmc_aspeed.c,
aspeed_kcs_inb & aspeed_kcs_outb.
>
>> +/* Different phases of the KCS BMC module */
>> +enum kcs_phases {
>> + /* BMC should not be expecting nor sending any data. */
>> + KCS_PHASE_IDLE,
> Perhaps kernel-doc?
Code + inline comments should be better than kernel-doc ? Or move it out
like :

/* The interface for checksum offload between the stack and networking
drivers
 * is as follows...
 *
 * A. IP checksum related features
 *
 * Drivers advertise checksum offload capabilities in the features of a
device.
 * From the stack's point of view these are capabilities offered by the
driver,
 * a driver typically only advertises features that it is capable of
offloading
 * to its device.
 *
 * The checksum related features are:
 *
 *    NETIF_F_HW_CSUM    - The driver (or its device) is able to
compute one
 *              IP (one's complement) checksum for any combination
 *              of protocols or protocol layering. The checksum is
 *              computed and set in a packet per the CHECKSUM_PARTIAL
 *              interface (see below).
 *
 *    NETIF_F_IP_CSUM - Driver (device) is only able to checksum plain
 *              TCP or UDP packets over IPv4. These are specifically
 *              unencapsulated packets of the form IPv4|TCP or
 *              IPv4|UDP where the Protocol field in the IPv4 header
 *              is TCP or UDP. The IPv4 header may contain IP options
 *              This feature cannot be set in features for a device
 *              with NETIF_F_HW_CSUM also set. This feature is being
 *              DEPRECATED (see below).
>> +};
>
>> +/* IPMI 2.0 - 9.5, KCS Interface Registers */
>> +struct kcs_ioreg {
>> + u32 idr; /* Input Data Register */
>> + u32 odr; /* Output Data Register */
>> + u32 str; /* Status Register */
> kernel-doc
>> +};
>> +
>> +static inline void *kcs_bmc_priv(const struct kcs_bmc *kcs_bmc)
>> +{
>> + return kcs_bmc->priv;
>> +}
>> +
>> +extern int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc);
>> +extern struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int
>> sizeof_priv,
>> + u32 channel);
> Drop extern.
After dropping extern, it truly passed compilation, have any special
reason to drop 'extern' ?
I saw in kernel still use extern like : extern void printk_nmi_init(void);
>> +#endif
> Next one could be reviewed when you split this patch to two.
Got it!

\
 
 \ /
  Last update: 2018-01-26 06:34    [W:0.445 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site