Re: [PATCH v10 06/12] peci: Add a PECI adapter driver for Aspeed AST24xx/AST25xx

[Jae Hyun Yoo]

On 1/14/2019 3:37 AM, Joel Stanley wrote:
> On Tue, 8 Jan 2019 at 08:11, Jae Hyun Yoo <jae.hyun.yoo@xxxxxxxxxxxxxxx> wrote:
> > +       ret = of_property_read_u32(priv->dev->of_node, "cmd-timeout-ms",
> > +                                  &priv->cmd_timeout_ms);
> > +       if (ret || priv->cmd_timeout_ms > PECI_CMD_TIMEOUT_MS_MAX ||
> > +           priv->cmd_timeout_ms == 0) {
> > +               if (!ret)
> > +                       dev_warn(priv->dev,
> > +                                "Invalid cmd-timeout-ms : %u. Use default : %u\n",
> > +                                priv->cmd_timeout_ms,
> > +                                PECI_CMD_TIMEOUT_MS_DEFAULT);
>
> As this property is documented as optional, I'd split out the checks
> so you only warn when the value provided is invalid.

Please check the above 'if' statement too. It prints out warning only
when the property is defined in device tree but the value is out of
range.

> > +
> > +       regmap_write(priv->regmap, ASPEED_PECI_CTRL,
> > +                    FIELD_PREP(PECI_CTRL_CLK_DIV_MASK, PECI_CLK_DIV_DEFAULT) |
> > +                    PECI_CTRL_PECI_CLK_EN);
> > +
> > +       /**
>
> Just the one *.

Will fix it.

> > +        * Timing negotiation period setting.
> > +        * The unit of the programmed value is 4 times of PECI clock period.
> > +        */
> > +       regmap_write(priv->regmap, ASPEED_PECI_TIMING,
> > +                    FIELD_PREP(PECI_TIMING_MESSAGE_MASK, msg_timing) |
> > +                    FIELD_PREP(PECI_TIMING_ADDRESS_MASK, addr_timing));
>
> > +static int aspeed_peci_xfer(struct peci_adapter *adapter,
> > +                           struct peci_xfer_msg *msg)
> > +{
> > +       struct aspeed_peci *priv = peci_get_adapdata(adapter);
> > +
> > +       return aspeed_peci_xfer_native(priv, msg);
> > +}
>
> It looks like you could do the peci_get_adapdata in
> aspeed_peci_xfer_native and drop the need for this wrapper.

Yes, that would be neater. Will remove this wrapper.

> > +
> > +static int aspeed_peci_probe(struct platform_device *pdev)
> > +{
>
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       base = devm_ioremap_resource(&pdev->dev, res);
> > +       if (IS_ERR(base)) {
> > +               ret = PTR_ERR(base);
> > +               goto err_put_adapter_dev;
> > +       }
> > +
> > +       priv->regmap = devm_regmap_init_mmio(&pdev->dev, base,
> > +                                            &aspeed_peci_regmap_config);
> > +       if (IS_ERR(priv->regmap)) {
> > +               ret = PTR_ERR(priv->regmap);
> > +               goto err_put_adapter_dev;
> > +       }
> > +
> > +       /**
> > +        * We check that the regmap works on this very first access,
> > +        * but as this is an MMIO-backed regmap, subsequent regmap
> > +        * access is not going to fail and we skip error checks from
> > +        * this point.
>
> Why do you use a regmap for this driver? AFAICT it has exclusive
> ownership over the register range it uses, which is sometimes a reason
> to use a regmap over a mmio region.
>
> I'm not sure if you've ever disassembled drivers/base/regmap/regmap.o,
> but if you do you will find that a single mmio read turns into
> hundreds of instructions.

No specific reason. regmap makes some overhead as you mentioned but it
also provides some advantages on access simplification, endianness
handling and register dump at run time. I would not insist using of
regmap if you prefer using of raw readl and writel. Do you want replace
regmap with readl and writel in this driver?

Thanks,
Jae

> Cheers,
>
> Joel