RE: [PATCH linux dev-4.10] ipmi: add an Aspeed KCS IPMI BMC driver

["Haiyue Wang" <haiyue.wang@linux.intel.com>]

Thanks C. 'linux-aspeed' should be a good reviewer. :)


Hi Andrew & Joel,

> @@ -94,8 +107,22 @@ lpc: lpc@1e789000 {
>         ranges = <0x0 0x1e789000 0x1000>;
>
>         lpc_bmc: lpc-bmc@0 {

Ah, the lpc-bmc node. This is Andrew's work, please add him to CC in the future so he can help review.

> -               compatible = "aspeed,ast2500-lpc-bmc";
> +               compatible = "aspeed,ast2500-lpc-bmc"; "simple-mfd", 
> + "syscon";

  >> Can you explain why we add the simple-mfd property here? The lpc device already has this, which enables us to have a regmap across the address space. If there's a further requirement then please explain it.
 
  Haiyue: I tried to remove this property, found that we had to add "simple-mfd" property into "aspeed,ast2500-lpc-bmc" node. If not, the nested node kcs1/2/3 can't be loaded successfully.

>                 reg = <0x0 0x80>;
> +               reg-io-width = <4>;
> +
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               ranges = <0x0 0x0 0x80>;
> +
> +               kcs3: kcs@3 {

 >> It's convention that the unit name has a corresponding reg property.
       
 >> It looks like you should use reg = <3>, which means you don't need the custom kcs_chan property.

 Haiyue: I added the ' reg = <0x0 0x80> ' property for memory/io space use, and use 'kcs_chan' for channel selection in kcs driver.

> +                       compatible = "aspeed,ast2500-kcs-bmc";
> +                       interrupts = <8>;
> +                       kcs_chan = <3>;
> +                       kcs_regs = <0x2C 0x38 0x44>;

I suggest placing these register offsets in the driver.

There are a number of channel specific offsets, registers and masks that need to be specified. From what I can see you specify some here in the device tree, and others are selected in the switch statements in your code. It's a good goal to make a code generic with the hardware differences specified in the device tree, but in this case it's not worth it.

I suggest a data structure that contains the registers, enable bits and mask, address bits and mask, etc. This way you can store a pointer to that data structure containing all of the information, and you don't need the switch statements in the code.

struct kcs_channel {
      u32 idr;
      u32 odr
      u32 str;
      u32 en_reg1;
      u32 en_mask1;
      u32 en_reg2;
      u32 en_mask2;
};

static const struct kcs_channels[4] =
  { .idr = 0x2C,
    .odr = 0x38,
    .str = 0x44,
    .en_reg1 = LPC_HICR2,
    .en_mask1 = LPC_HICR2_IBFIF3

etc.

Haiyue: This makes code and device tree more clean, I over-used the device tree. :-). And I defined bellowing, and kept the enable/disable channel
as before. Because you can see that enable / disable have the register setting with different order, and registers number are different also. So for
making the code clean, and let people easily understand the code by checking data sheet, I kept this. :)

/* IPMI 2.0 : 9.5 - KCS Interface Registers */
struct kcs_ioreg {
	u32 idr;
	u32 odr;
	u32 str;
};

static const struct kcs_ioreg kcs_channel_ioregs[KCS_CHANNEL_MAX] = {
	{ .idr = 0x24, .odr = 0x30, .str =  0x3C },
	{ .idr = 0x28, .odr = 0x34, .str =  0x40 },
	{ .idr = 0x2C, .odr = 0x38, .str =  0x44 },
	{ .idr = 0x94, .odr = 0x98, .str =  0x9C },
};

static void kcs_enable_channel(struct kcs_bmc *kcs_bmc, int enable)
{
	switch (kcs_bmc->chan) {
	case 1:
		if (enable) {
			regmap_update_bits(kcs_bmc->map, LPC_HICR2,
					LPC_HICR2_IBFIF1, LPC_HICR2_IBFIF1);
			regmap_update_bits(kcs_bmc->map, LPC_HICR0,
					LPC_HICR0_LPC1E, LPC_HICR0_LPC1E);
		} else {
			regmap_update_bits(kcs_bmc->map, LPC_HICR0,
					LPC_HICR0_LPC1E, 0);
			regmap_update_bits(kcs_bmc->map, LPC_HICR2,
					LPC_HICR2_IBFIF1, 0);
		}
		break;

	case 2:
		if (enable) {
			regmap_update_bits(kcs_bmc->map, LPC_HICR2,
					LPC_HICR2_IBFIF2, LPC_HICR2_IBFIF2);
			regmap_update_bits(kcs_bmc->map, LPC_HICR0,
					LPC_HICR0_LPC2E, LPC_HICR0_LPC2E);
		} else {
			regmap_update_bits(kcs_bmc->map, LPC_HICR0,
					LPC_HICR0_LPC2E, 0);
			regmap_update_bits(kcs_bmc->map, LPC_HICR2,
					LPC_HICR2_IBFIF2, 0);
		}
		break;

	case 3:
		if (enable) {
			regmap_update_bits(kcs_bmc->map, LPC_HICR2,
					LPC_HICR2_IBFIF3, LPC_HICR2_IBFIF3);
			regmap_update_bits(kcs_bmc->map, LPC_HICR0,
					LPC_HICR0_LPC3E, LPC_HICR0_LPC3E);
			regmap_update_bits(kcs_bmc->map, LPC_HICR4,
					LPC_HICR4_KCSENBL, LPC_HICR4_KCSENBL);
		} else {
			regmap_update_bits(kcs_bmc->map, LPC_HICR0,
					LPC_HICR0_LPC3E, 0);
			regmap_update_bits(kcs_bmc->map, LPC_HICR4,
					LPC_HICR4_KCSENBL, 0);
			regmap_update_bits(kcs_bmc->map, LPC_HICR2,
					LPC_HICR2_IBFIF3, 0);
		}
		break;

	case 4:
		if (enable) {
			regmap_update_bits(kcs_bmc->map, LPC_HICRB,
					LPC_HICRB_IBFIF4 | LPC_HICRB_LPC4E,
					LPC_HICRB_IBFIF4 | LPC_HICRB_LPC4E);
		} else {
			regmap_update_bits(kcs_bmc->map, LPC_HICRB,
					LPC_HICRB_IBFIF4 | LPC_HICRB_LPC4E,
					0);
		}
		break;

	default:
		break;
	}
}

BR,
Haiyue




-----Original Message-----
From: Cédric Le Goater [mailto:clg@kaod.org] 
Sent: Friday, December 8, 2017 16:12
To: Joel Stanley <joel@jms.id.au>; Haiyue Wang <haiyue.wang@linux.intel.com>
Cc: Andrew Jeffery <andrew@aj.id.au>; Jeremy Kerr <jk@ozlabs.org>; OpenBMC Maillist <openbmc@lists.ozlabs.org>
Subject: Re: [PATCH linux dev-4.10] ipmi: add an Aspeed KCS IPMI BMC driver

On 12/08/2017 07:01 AM, Joel Stanley wrote:
> On Fri, Dec 8, 2017 at 4:24 PM, Haiyue Wang <haiyue.wang@linux.intel.com> wrote:
>> Great notes, will update a new patch later, thanks Joel.
>>
>> I implemented the kcs by referenced the upstream bt driver. And the 
>> upstream maintainer is as bellowing, I hope the openbmc expert to 
>> review my patch firstly, will send to the maintainer soon. :-)
>>
>> ./scripts/get_maintainer.pl drivers/char/ipmi/bt-bmc.c Corey Minyard 
>> <minyard@acm.org> (supporter:IPMI SUBSYSTEM) 
>> openipmi-developer@lists.sourceforge.net (moderated list:IPMI 
>> SUBSYSTEM) linux-kernel@vger.kernel.org (open list)
> 
> Yep, add to that list:
> 
> openbmc@lists.ozlabs.org
> clg@fr.ibm.com
> jk@ozlabs.org
> andrew@aj.id.au
> joel@jms.id.au

and what about the linux-aspeed@lists.ozlabs.org ? 

C.

> In the past we have spent a long time pre-reviewing on the openbmc 
> list, and then had another extensive review process on the upstream 
> list. I hope it will help us get the code in a mergeable state faster 
> by doing the work upstream, with the OpenBMC team providing input.
> 
> Cheers,
> 
> Joel
>