[U-Boot] [PATCH 4/7] dm: Expand the uclass for Peripheral Controller Hubs (PCH)

[Simon Glass]

Hi Bin,

On 17 December 2015 at 03:09, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Simon,
>
> On Thu, Dec 17, 2015 at 12:09 PM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Bin,
>>
>> On 8 December 2015 at 06:23, Bin Meng <bmeng.cn at gmail.com> wrote:
>>> Hi Simon,
>>>
>>> On Tue, Dec 1, 2015 at 12:11 PM, Simon Glass <sjg at chromium.org> wrote:
>>>> A Peripheral Controller Hub is an Intel concept - it is like the peripherals
>>>
>>> I believe the name is Platform Controller Hub.
>>>
>>>> on an SoC and is often in a separate chip from the CPU. Even when it is not
>>>> it is addressed and used differently. The chip is typically found on the
>>>
>>> "Even when it is not" (a separate chip) "it is addressed and used
>>> differently"? I feel it should be "it is addressed and used the same'?
>>>
>>>> first PCI device.
>>>
>>> This indicates b.d.f = 0.0.0, but for registers like RCBA, SPI base,
>>> those are actually on the LPC device (b.d.f = 0.1f.0). Maybe we can
>>> say: the chip is typically found on the first PCI bus and integrates
>>> multiple devices?
>>>
>>>>
>>>> We have a very simple uclass to support PCHs. Add a few operations, such as
>>>> setting up the devices on the PCH and finding the SPI controller base
>>>> address. Also move it into drivers/pch/ since we will be adding a few PCH
>>>> drivers.
>>>>
>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>> ---
>>>>
>>>>  arch/x86/lib/Makefile                      |  1 -
>>>>  drivers/Makefile                           |  1 +
>>>>  drivers/pch/Makefile                       |  5 +++
>>>>  {arch/x86/lib => drivers/pch}/pch-uclass.c | 32 +++++++++++++++
>>>>  include/pch.h                              | 66 ++++++++++++++++++++++++++++++
>>>>  5 files changed, 104 insertions(+), 1 deletion(-)
>>>>  create mode 100644 drivers/pch/Makefile
>>>>  rename {arch/x86/lib => drivers/pch}/pch-uclass.c (53%)
>>>>  create mode 100644 include/pch.h
>>>>
>>>> diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile
>>>> index cd5ecb6..43792bc 100644
>>>> --- a/arch/x86/lib/Makefile
>>>> +++ b/arch/x86/lib/Makefile
>>>> @@ -24,7 +24,6 @@ obj-$(CONFIG_I8254_TIMER) += i8254.o
>>>>  ifndef CONFIG_DM_PCI
>>>>  obj-$(CONFIG_PCI) += pci_type1.o
>>>>  endif
>>>> -obj-y  += pch-uclass.o
>>>>  obj-y  += pirq_routing.o
>>>>  obj-y  += relocate.o
>>>>  obj-y += physmem.o
>>>> diff --git a/drivers/Makefile b/drivers/Makefile
>>>> index c9031f2..acc6af9 100644
>>>> --- a/drivers/Makefile
>>>> +++ b/drivers/Makefile
>>>> @@ -51,6 +51,7 @@ obj-y += hwmon/
>>>>  obj-y += misc/
>>>>  obj-y += pcmcia/
>>>>  obj-y += dfu/
>>>> +obj-$(CONFIG_X86) += pch/
>>>>  obj-y += rtc/
>>>>  obj-y += sound/
>>>>  obj-y += timer/
>>>> diff --git a/drivers/pch/Makefile b/drivers/pch/Makefile
>>>> new file mode 100644
>>>> index 0000000..d69a99c
>>>> --- /dev/null
>>>> +++ b/drivers/pch/Makefile
>>>> @@ -0,0 +1,5 @@
>>>> +#
>>>> +# SPDX-License-Identifier:     GPL-2.0+
>>>> +#
>>>> +
>>>> +obj-y += pch-uclass.o
>>>> diff --git a/arch/x86/lib/pch-uclass.c b/drivers/pch/pch-uclass.c
>>>> similarity index 53%
>>>> rename from arch/x86/lib/pch-uclass.c
>>>> rename to drivers/pch/pch-uclass.c
>>>> index 20dfa81..09a0107 100644
>>>> --- a/arch/x86/lib/pch-uclass.c
>>>> +++ b/drivers/pch/pch-uclass.c
>>>> @@ -7,10 +7,42 @@
>>>>
>>>>  #include <common.h>
>>>>  #include <dm.h>
>>>> +#include <pch.h>
>>>>  #include <dm/root.h>
>>>>
>>>>  DECLARE_GLOBAL_DATA_PTR;
>>>>
>>>> +int pch_init(struct udevice *dev)
>>>> +{
>>>> +       struct pch_ops *ops = pch_get_ops(dev);
>>>> +
>>>> +       if (!ops->init)
>>>> +               return -ENOSYS;
>>>> +
>>>> +       return ops->init(dev);
>>>> +}
>>>> +
>>>> +int pch_get_sbase(struct udevice *dev, ulong *sbasep)
>>>> +{
>>>> +       struct pch_ops *ops = pch_get_ops(dev);
>>>> +
>>>> +       *sbasep = 0;
>>>> +       if (!ops->get_sbase)
>>>> +               return -ENOSYS;
>>>> +
>>>> +       return ops->get_sbase(dev, sbasep);
>>>> +}
>>>> +
>>>> +int pch_get_version(struct udevice *dev)
>>>> +{
>>>> +       struct pch_ops *ops = pch_get_ops(dev);
>>>> +
>>>> +       if (!ops->get_version)
>>>> +               return -ENOSYS;
>>>> +
>>>> +       return ops->get_version(dev);
>>>> +}
>>>> +
>>>>  static int pch_uclass_post_bind(struct udevice *bus)
>>>>  {
>>>>         /*
>>>> diff --git a/include/pch.h b/include/pch.h
>>>> new file mode 100644
>>>> index 0000000..98bb5f2
>>>> --- /dev/null
>>>> +++ b/include/pch.h
>>>> @@ -0,0 +1,66 @@
>>>> +/*
>>>> + * Copyright (c) 2015 Google, Inc
>>>> + * Written by Simon Glass <sjg at chromium.org>
>>>> + *
>>>> + * SPDX-License-Identifier:    GPL-2.0+
>>>> + */
>>>> +
>>>> +#ifndef __pch_h
>>>> +#define __pch_h
>>>> +
>>>> +struct pch_ops {
>>>> +       /**
>>>> +        * init() - set up the PCH devices
>>>> +        *
>>>> +        * This makes sure that all the devices are ready for use. They are
>>>> +        * not actually started, just set up so that they can be probed.
>>>> +        */
>>>> +       int (*init)(struct udevice *dev);
>>>
>>> Do we need create such an init op? Should this be done in the driver's
>>> probe routine?
>>
>> The PCH is modelled in ivybridge as the device at address 0,0,0. I
>> have found that we need to do the init in two stages, so this is the
>> reason for the init() method. However, I am still working on
>> refactoring and simplifying the code. So it is possible that at some
>> point I will figure out how to remove this. But for now I cannot see
>> how.

This comment is incorrect - it is the northbridge at is at 0,0,0. The
PCH is 0,1f,0.

>>
>
> Does if (gd->flags & GD_FLG_RELOC) not help?

I already use that, but no it does not help. That said, I do hope to
remove it. It's very difficult to move everything at once and it would
make the code difficult to review also.

Regards,
Simon