| Message ID | 1433688642-19861-8-git-send-email-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Simon, On Sun, Jun 7, 2015 at 10:50 PM, Simon Glass <sjg@chromium.org> wrote: > This driver should use the x86 PCI configuration functions. Also adjust its > compatible string to something generic (i.e. without a vendor name). > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > drivers/pci/pci_x86.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/pci_x86.c b/drivers/pci/pci_x86.c > index 901bdca..9f842c3 100644 > --- a/drivers/pci/pci_x86.c > +++ b/drivers/pci/pci_x86.c > @@ -7,12 +7,15 @@ > #include <common.h> > #include <dm.h> > #include <pci.h> > +#include <asm/pci.h> > > static const struct dm_pci_ops x86_pci_ops = { To keep the consistent naming to match the driver name, can we rename this to pci_x86_ops? > + .read_config = pci_x86_read_config, > + .write_config = pci_x86_write_config, Can we move pci_x86_read_config() and pci_x86_write_config() from arch/x86/cpu/pci.c to this file to make it a complete driver file? Also create a new header file pci_x86.h to declare these two so that it can be used by ivybridge. > }; > > static const struct udevice_id x86_pci_ids[] = { Can we rename this to pci_x86_ids? > - { .compatible = "x86,pci" }, > + { .compatible = "pci-x86" }, > { } > }; > > -- Regards, Bin
Hi Bin, On 7 June 2015 at 20:15, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Simon, > > On Sun, Jun 7, 2015 at 10:50 PM, Simon Glass <sjg@chromium.org> wrote: >> This driver should use the x86 PCI configuration functions. Also adjust its >> compatible string to something generic (i.e. without a vendor name). >> >> Signed-off-by: Simon Glass <sjg@chromium.org> >> --- >> >> drivers/pci/pci_x86.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pci/pci_x86.c b/drivers/pci/pci_x86.c >> index 901bdca..9f842c3 100644 >> --- a/drivers/pci/pci_x86.c >> +++ b/drivers/pci/pci_x86.c >> @@ -7,12 +7,15 @@ >> #include <common.h> >> #include <dm.h> >> #include <pci.h> >> +#include <asm/pci.h> >> >> static const struct dm_pci_ops x86_pci_ops = { > > To keep the consistent naming to match the driver name, can we rename > this to pci_x86_ops? OK > >> + .read_config = pci_x86_read_config, >> + .write_config = pci_x86_write_config, > > Can we move pci_x86_read_config() and pci_x86_write_config() from > arch/x86/cpu/pci.c to this file to make it a complete driver file? > Also create a new header file pci_x86.h to declare these two so that > it can be used by ivybridge. I can certainly drop the ivybridge duplication. But I don't think it is right to call directly into a driver in drivers/... We should use driver model for this if we want to do it properly. I would like to continue the work to move x86 fully to driver model. In the meantime I think that directly called functions should be in arch/x86. > >> }; >> >> static const struct udevice_id x86_pci_ids[] = { > > Can we rename this to pci_x86_ids? OK > >> - { .compatible = "x86,pci" }, >> + { .compatible = "pci-x86" }, >> { } >> }; >> >> -- > Regards, Simon
Hi Simon, On Wed, Jun 24, 2015 at 11:18 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Bin, > > On 7 June 2015 at 20:15, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Simon, >> >> On Sun, Jun 7, 2015 at 10:50 PM, Simon Glass <sjg@chromium.org> wrote: >>> This driver should use the x86 PCI configuration functions. Also adjust its >>> compatible string to something generic (i.e. without a vendor name). >>> >>> Signed-off-by: Simon Glass <sjg@chromium.org> >>> --- >>> >>> drivers/pci/pci_x86.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/pci/pci_x86.c b/drivers/pci/pci_x86.c >>> index 901bdca..9f842c3 100644 >>> --- a/drivers/pci/pci_x86.c >>> +++ b/drivers/pci/pci_x86.c >>> @@ -7,12 +7,15 @@ >>> #include <common.h> >>> #include <dm.h> >>> #include <pci.h> >>> +#include <asm/pci.h> >>> >>> static const struct dm_pci_ops x86_pci_ops = { >> >> To keep the consistent naming to match the driver name, can we rename >> this to pci_x86_ops? > > OK > >> >>> + .read_config = pci_x86_read_config, >>> + .write_config = pci_x86_write_config, >> >> Can we move pci_x86_read_config() and pci_x86_write_config() from >> arch/x86/cpu/pci.c to this file to make it a complete driver file? >> Also create a new header file pci_x86.h to declare these two so that >> it can be used by ivybridge. > > I can certainly drop the ivybridge duplication. But I don't think it > is right to call directly into a driver in drivers/... > > We should use driver model for this if we want to do it properly. I > would like to continue the work to move x86 fully to driver model. > > In the meantime I think that directly called functions should be in arch/x86. > Sorry I don't get it. I mean moving the implementation of pci_x86_read_config() and pci_x86_write_config() to drivers/pci/pci_x86.c. Do you have some concern about this? [snip] Regards, Bin
Hi Bin, On 23 June 2015 at 21:46, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Simon, > > On Wed, Jun 24, 2015 at 11:18 AM, Simon Glass <sjg@chromium.org> wrote: >> Hi Bin, >> >> On 7 June 2015 at 20:15, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Hi Simon, >>> >>> On Sun, Jun 7, 2015 at 10:50 PM, Simon Glass <sjg@chromium.org> wrote: >>>> This driver should use the x86 PCI configuration functions. Also adjust its >>>> compatible string to something generic (i.e. without a vendor name). >>>> >>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>> --- >>>> >>>> drivers/pci/pci_x86.c | 5 ++++- >>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/pci/pci_x86.c b/drivers/pci/pci_x86.c >>>> index 901bdca..9f842c3 100644 >>>> --- a/drivers/pci/pci_x86.c >>>> +++ b/drivers/pci/pci_x86.c >>>> @@ -7,12 +7,15 @@ >>>> #include <common.h> >>>> #include <dm.h> >>>> #include <pci.h> >>>> +#include <asm/pci.h> >>>> >>>> static const struct dm_pci_ops x86_pci_ops = { >>> >>> To keep the consistent naming to match the driver name, can we rename >>> this to pci_x86_ops? >> >> OK >> >>> >>>> + .read_config = pci_x86_read_config, >>>> + .write_config = pci_x86_write_config, >>> >>> Can we move pci_x86_read_config() and pci_x86_write_config() from >>> arch/x86/cpu/pci.c to this file to make it a complete driver file? >>> Also create a new header file pci_x86.h to declare these two so that >>> it can be used by ivybridge. >> >> I can certainly drop the ivybridge duplication. But I don't think it >> is right to call directly into a driver in drivers/... >> >> We should use driver model for this if we want to do it properly. I >> would like to continue the work to move x86 fully to driver model. >> >> In the meantime I think that directly called functions should be in arch/x86. >> > > Sorry I don't get it. I mean moving the implementation of > pci_x86_read_config() and pci_x86_write_config() to > drivers/pci/pci_x86.c. Do you have some concern about this? > > [snip] Yes it is still used by arch/x86/cpu/coreboot/pci.c - and as I say I don't like the 'call directly into driver' idea. If we could remove the coreboot case then it would be fine. Regards, Simon
Hi Simon, On Wed, Jun 24, 2015 at 11:54 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Bin, > > On 23 June 2015 at 21:46, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Simon, >> >> On Wed, Jun 24, 2015 at 11:18 AM, Simon Glass <sjg@chromium.org> wrote: >>> Hi Bin, >>> >>> On 7 June 2015 at 20:15, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> Hi Simon, >>>> >>>> On Sun, Jun 7, 2015 at 10:50 PM, Simon Glass <sjg@chromium.org> wrote: >>>>> This driver should use the x86 PCI configuration functions. Also adjust its >>>>> compatible string to something generic (i.e. without a vendor name). >>>>> >>>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>>> --- >>>>> >>>>> drivers/pci/pci_x86.c | 5 ++++- >>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/pci/pci_x86.c b/drivers/pci/pci_x86.c >>>>> index 901bdca..9f842c3 100644 >>>>> --- a/drivers/pci/pci_x86.c >>>>> +++ b/drivers/pci/pci_x86.c >>>>> @@ -7,12 +7,15 @@ >>>>> #include <common.h> >>>>> #include <dm.h> >>>>> #include <pci.h> >>>>> +#include <asm/pci.h> >>>>> >>>>> static const struct dm_pci_ops x86_pci_ops = { >>>> >>>> To keep the consistent naming to match the driver name, can we rename >>>> this to pci_x86_ops? >>> >>> OK >>> >>>> >>>>> + .read_config = pci_x86_read_config, >>>>> + .write_config = pci_x86_write_config, >>>> >>>> Can we move pci_x86_read_config() and pci_x86_write_config() from >>>> arch/x86/cpu/pci.c to this file to make it a complete driver file? >>>> Also create a new header file pci_x86.h to declare these two so that >>>> it can be used by ivybridge. >>> >>> I can certainly drop the ivybridge duplication. But I don't think it >>> is right to call directly into a driver in drivers/... >>> >>> We should use driver model for this if we want to do it properly. I >>> would like to continue the work to move x86 fully to driver model. >>> >>> In the meantime I think that directly called functions should be in arch/x86. >>> >> >> Sorry I don't get it. I mean moving the implementation of >> pci_x86_read_config() and pci_x86_write_config() to >> drivers/pci/pci_x86.c. Do you have some concern about this? >> >> [snip] > > Yes it is still used by arch/x86/cpu/coreboot/pci.c - and as I say I > don't like the 'call directly into driver' idea. If we could remove > the coreboot case then it would be fine. Why not we drop this coreboot pci driver (arch/x86/cpu/coreboot/pci.c) and use the new one (drivers/pci/pci_x86.c) directly? Regards, Bin
Hi Bin, On 23 June 2015 at 21:59, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Simon, > > On Wed, Jun 24, 2015 at 11:54 AM, Simon Glass <sjg@chromium.org> wrote: >> Hi Bin, >> >> On 23 June 2015 at 21:46, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Hi Simon, >>> >>> On Wed, Jun 24, 2015 at 11:18 AM, Simon Glass <sjg@chromium.org> wrote: >>>> Hi Bin, >>>> >>>> On 7 June 2015 at 20:15, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>> Hi Simon, >>>>> >>>>> On Sun, Jun 7, 2015 at 10:50 PM, Simon Glass <sjg@chromium.org> wrote: >>>>>> This driver should use the x86 PCI configuration functions. Also adjust its >>>>>> compatible string to something generic (i.e. without a vendor name). >>>>>> >>>>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>>>> --- >>>>>> >>>>>> drivers/pci/pci_x86.c | 5 ++++- >>>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/pci/pci_x86.c b/drivers/pci/pci_x86.c >>>>>> index 901bdca..9f842c3 100644 >>>>>> --- a/drivers/pci/pci_x86.c >>>>>> +++ b/drivers/pci/pci_x86.c >>>>>> @@ -7,12 +7,15 @@ >>>>>> #include <common.h> >>>>>> #include <dm.h> >>>>>> #include <pci.h> >>>>>> +#include <asm/pci.h> >>>>>> >>>>>> static const struct dm_pci_ops x86_pci_ops = { >>>>> >>>>> To keep the consistent naming to match the driver name, can we rename >>>>> this to pci_x86_ops? >>>> >>>> OK >>>> >>>>> >>>>>> + .read_config = pci_x86_read_config, >>>>>> + .write_config = pci_x86_write_config, >>>>> >>>>> Can we move pci_x86_read_config() and pci_x86_write_config() from >>>>> arch/x86/cpu/pci.c to this file to make it a complete driver file? >>>>> Also create a new header file pci_x86.h to declare these two so that >>>>> it can be used by ivybridge. >>>> >>>> I can certainly drop the ivybridge duplication. But I don't think it >>>> is right to call directly into a driver in drivers/... >>>> >>>> We should use driver model for this if we want to do it properly. I >>>> would like to continue the work to move x86 fully to driver model. >>>> >>>> In the meantime I think that directly called functions should be in arch/x86. >>>> >>> >>> Sorry I don't get it. I mean moving the implementation of >>> pci_x86_read_config() and pci_x86_write_config() to >>> drivers/pci/pci_x86.c. Do you have some concern about this? >>> >>> [snip] >> >> Yes it is still used by arch/x86/cpu/coreboot/pci.c - and as I say I >> don't like the 'call directly into driver' idea. If we could remove >> the coreboot case then it would be fine. > > Why not we drop this coreboot pci driver (arch/x86/cpu/coreboot/pci.c) > and use the new one (drivers/pci/pci_x86.c) directly? Yes let's do that. I can't see any reason not to. Regards, Simon
Hi Bin, On 24 June 2015 at 09:15, Simon Glass <sjg@chromium.org> wrote: > Hi Bin, > > On 23 June 2015 at 21:59, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Simon, >> >> On Wed, Jun 24, 2015 at 11:54 AM, Simon Glass <sjg@chromium.org> wrote: >>> Hi Bin, >>> >>> On 23 June 2015 at 21:46, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> Hi Simon, >>>> >>>> On Wed, Jun 24, 2015 at 11:18 AM, Simon Glass <sjg@chromium.org> wrote: >>>>> Hi Bin, >>>>> >>>>> On 7 June 2015 at 20:15, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>> Hi Simon, >>>>>> >>>>>> On Sun, Jun 7, 2015 at 10:50 PM, Simon Glass <sjg@chromium.org> wrote: >>>>>>> This driver should use the x86 PCI configuration functions. Also adjust its >>>>>>> compatible string to something generic (i.e. without a vendor name). >>>>>>> >>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>>>>> --- >>>>>>> >>>>>>> drivers/pci/pci_x86.c | 5 ++++- >>>>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/pci/pci_x86.c b/drivers/pci/pci_x86.c >>>>>>> index 901bdca..9f842c3 100644 >>>>>>> --- a/drivers/pci/pci_x86.c >>>>>>> +++ b/drivers/pci/pci_x86.c >>>>>>> @@ -7,12 +7,15 @@ >>>>>>> #include <common.h> >>>>>>> #include <dm.h> >>>>>>> #include <pci.h> >>>>>>> +#include <asm/pci.h> >>>>>>> >>>>>>> static const struct dm_pci_ops x86_pci_ops = { >>>>>> >>>>>> To keep the consistent naming to match the driver name, can we rename >>>>>> this to pci_x86_ops? >>>>> >>>>> OK >>>>> >>>>>> >>>>>>> + .read_config = pci_x86_read_config, >>>>>>> + .write_config = pci_x86_write_config, >>>>>> >>>>>> Can we move pci_x86_read_config() and pci_x86_write_config() from >>>>>> arch/x86/cpu/pci.c to this file to make it a complete driver file? >>>>>> Also create a new header file pci_x86.h to declare these two so that >>>>>> it can be used by ivybridge. >>>>> >>>>> I can certainly drop the ivybridge duplication. But I don't think it >>>>> is right to call directly into a driver in drivers/... >>>>> >>>>> We should use driver model for this if we want to do it properly. I >>>>> would like to continue the work to move x86 fully to driver model. >>>>> >>>>> In the meantime I think that directly called functions should be in arch/x86. >>>>> >>>> >>>> Sorry I don't get it. I mean moving the implementation of >>>> pci_x86_read_config() and pci_x86_write_config() to >>>> drivers/pci/pci_x86.c. Do you have some concern about this? >>>> >>>> [snip] >>> >>> Yes it is still used by arch/x86/cpu/coreboot/pci.c - and as I say I >>> don't like the 'call directly into driver' idea. If we could remove >>> the coreboot case then it would be fine. >> >> Why not we drop this coreboot pci driver (arch/x86/cpu/coreboot/pci.c) >> and use the new one (drivers/pci/pci_x86.c) directly? > > Yes let's do that. I can't see any reason not to. I think the problem is that cpu/ivybridge/pci.c has its own driver and wants to call the pci_x86_read/write_config() functions. So they really need to be in a generic area. I was thinking about coreboot yesterday, but the problem is ivybridge. Regards, Simon
diff --git a/drivers/pci/pci_x86.c b/drivers/pci/pci_x86.c index 901bdca..9f842c3 100644 --- a/drivers/pci/pci_x86.c +++ b/drivers/pci/pci_x86.c @@ -7,12 +7,15 @@ #include <common.h> #include <dm.h> #include <pci.h> +#include <asm/pci.h> static const struct dm_pci_ops x86_pci_ops = { + .read_config = pci_x86_read_config, + .write_config = pci_x86_write_config, }; static const struct udevice_id x86_pci_ids[] = { - { .compatible = "x86,pci" }, + { .compatible = "pci-x86" }, { } };
This driver should use the x86 PCI configuration functions. Also adjust its compatible string to something generic (i.e. without a vendor name). Signed-off-by: Simon Glass <sjg@chromium.org> --- drivers/pci/pci_x86.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)