[U-Boot] [PATCH 06/57] x86: ivybridge: Set up the LPC device using driver model

[Simon Glass]

Hi Bin,

On 17 December 2015 at 03:00, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Simon,
>
> On Wed, Dec 16, 2015 at 2:57 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Bin,
>>
>> On 13 December 2015 at 05:52, Bin Meng <bmeng.cn at gmail.com> wrote:
>>> Hi Simon,
>>>
>>> On Tue, Dec 8, 2015 at 11:38 AM, Simon Glass <sjg at chromium.org> wrote:
>>>> Find the LPC device in arch_cpu_init_dm() as a first step to converting
>>>> this code to use driver model. Probing the LPC will probe its parent (the
>>>> PCH) automatically, so make sure that probing the PCH does nothing before
>>>> relocation.
>>>>
>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>> ---
>>>>
>>>>  arch/x86/cpu/ivybridge/bd82x6x.c | 3 +++
>>>>  arch/x86/cpu/ivybridge/cpu.c     | 6 +++++-
>>>>  arch/x86/cpu/ivybridge/lpc.c     | 6 ++++++
>>>>  arch/x86/dts/chromebook_link.dts | 1 +
>>>>  4 files changed, 15 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/x86/cpu/ivybridge/bd82x6x.c b/arch/x86/cpu/ivybridge/bd82x6x.c
>>>> index abd59da..be39bcd 100644
>>>> --- a/arch/x86/cpu/ivybridge/bd82x6x.c
>>>> +++ b/arch/x86/cpu/ivybridge/bd82x6x.c
>>>> @@ -64,6 +64,9 @@ static int bd82x6x_probe(struct udevice *dev)
>>>>         int sata_node, gma_node;
>>>>         int ret;
>>>>
>>>> +       if (!(gd->flags & GD_FLG_RELOC))
>>>> +               return 0;
>>>> +
>>>>         hose = pci_bus_to_hose(0);
>>>>         lpc_enable(PCH_LPC_DEV);
>>>>         lpc_init(hose, PCH_LPC_DEV);
>>>> diff --git a/arch/x86/cpu/ivybridge/cpu.c b/arch/x86/cpu/ivybridge/cpu.c
>>>> index 0387444..fd7e1fc 100644
>>>> --- a/arch/x86/cpu/ivybridge/cpu.c
>>>> +++ b/arch/x86/cpu/ivybridge/cpu.c
>>>> @@ -126,7 +126,7 @@ int arch_cpu_init_dm(void)
>>>>  {
>>>>         const void *blob = gd->fdt_blob;
>>>>         struct pci_controller *hose;
>>>> -       struct udevice *bus;
>>>> +       struct udevice *bus, *dev;
>>>>         int node;
>>>>         int ret;
>>>>
>>>> @@ -141,6 +141,10 @@ int arch_cpu_init_dm(void)
>>>>         /* TODO(sjg at chromium.org): Get rid of gd->hose */
>>>>         gd->hose = hose;
>>>>
>>>> +       ret = uclass_first_device(UCLASS_LPC, &dev);
>>>> +       if (!dev)
>>>> +               return -ENODEV;
>>>> +
>>>>         node = fdtdec_next_compatible(blob, 0, COMPAT_INTEL_PCH);
>>>>         if (node < 0)
>>>>                 return -ENOENT;
>>>> diff --git a/arch/x86/cpu/ivybridge/lpc.c b/arch/x86/cpu/ivybridge/lpc.c
>>>> index 3efd3e8..04a7451 100644
>>>> --- a/arch/x86/cpu/ivybridge/lpc.c
>>>> +++ b/arch/x86/cpu/ivybridge/lpc.c
>>>> @@ -568,6 +568,11 @@ void lpc_enable(pci_dev_t dev)
>>>>         setbits_le32(RCB_REG(FD2), PCH_ENABLE_DBDF);
>>>>  }
>>>>
>>>> +static int bd82x6x_lpc_probe(struct udevice *dev)
>>>> +{
>>>> +       return 0;
>>>> +}
>>>> +
>>>>  static const struct udevice_id bd82x6x_lpc_ids[] = {
>>>>         { .compatible = "intel,bd82x6x-lpc" },
>>>>         { }
>>>> @@ -577,4 +582,5 @@ U_BOOT_DRIVER(bd82x6x_lpc_drv) = {
>>>>         .name           = "lpc",
>>>>         .id             = UCLASS_LPC,
>>>>         .of_match       = bd82x6x_lpc_ids,
>>>> +       .probe          = bd82x6x_lpc_probe,
>>>>  };
>>>> diff --git a/arch/x86/dts/chromebook_link.dts b/arch/x86/dts/chromebook_link.dts
>>>> index 4d158da..7a009db 100644
>>>> --- a/arch/x86/dts/chromebook_link.dts
>>>> +++ b/arch/x86/dts/chromebook_link.dts
>>>> @@ -223,6 +223,7 @@
>>>>                                 compatible = "intel,bd82x6x-lpc";
>>>>                                 #address-cells = <1>;
>>>>                                 #size-cells = <0>;
>>>> +                               u-boot,dm-pre-reloc;
>>>>                                 cros-ec at 200 {
>>>>                                         compatible = "google,cros-ec";
>>>>                                         reg = <0x204 1 0x200 1 0x880 0x80>;
>>>> --
>>>
>>> The codes look good to me, but I have one question: what is the LPC
>>> uclass for? My understanding is that we already have the PCH uclass,
>>> which is for the bridge. LPC uclass seems to be duplicated. We can
>>> have cros-ec directly attached to the PCH node in the device tree.
>>
>> I was going to mention that in the cover letter.
>>
>> At present I have the northbridge as 0,0,0 device. The PCH is at
>> 0,1f,0. Looking at the Intel datasheets the LPC is one of the pieces
>> in the PCH. SPI is another piece. So I came up with having the PCH as
>> the parent device and LPC and SPI as children.
>>
>
> But LPC is using the same PCI configuration space registers (b.d.f =
> 0.1f.0) as PCH for the programming interface, not like SPI which is
> I/O space. With LPC uclass, we put LPC as the child node of the PCH
> node, so if we access LPC configuration space registers, we end up
> using LPC's parent device as the parameter for the DM PCI APIs. This
> looks odd to me.
>
> This unfortunately affects IRQ router as well, if we put IRQ router as
> the child node under PCH. Can we just merge all of these into one PCH
> uclass?

Conceptually I think we should have different devices in their own
uclass. It seems wrong to put several drivers in together just because
they are addressed in the same place.

I too have struggled with the idea of a device that appears on
multiple PCI addresses - it does not fit well with driver model, where
we assume that one driver is at one device. But IMO we should model
the PCH as a collection of child devices - using dev->parent is easy
enough and it's pretty clear what is going on. We could implement PCI
access as methods in the PCH uclass but I don't think it is worth it
for now.

Regards,
Simon