Re: [PATCH 2/2] net, thunder, bgx: Add support for ACPI binding.

Re: [PATCH 2/2] net, thunder, bgx: Add support for ACPI binding.

[Mark Rutland]

[Correcting the devicetree list address, which I typo'd in my original
reply]

> >> +static const char * const addr_propnames[] = {
> >> +	"mac-address",
> >> +	"local-mac-address",
> >> +	"address",
> >> +};
> >
> > If these are going to be generally necessary, then we should get them
> > adopted as standardised _DSD properties (ideally just one of them).
> 
> As far as I can tell, and please correct me if I am wrong, ACPI-6.0 
> doesn't contemplate MAC addresses.
> 
> Today we are using "mac-address", which is an Integer containing the MAC 
> address in its lowest order 48 bits in Little-Endian byte order.
> 
> The hardware and ACPI tables are here today, and we would like to 
> support it.  If some future ACPI specification specifies a standard way 
> to do this, we will probably adapt the code to do this in a standard manner.
> 
> 
> >
> > [...]
> >
> >> +static acpi_status bgx_acpi_register_phy(acpi_handle handle,
> >> +					 u32 lvl, void *context, void **rv)
> >> +{
> >> +	struct acpi_reference_args args;
> >> +	const union acpi_object *prop;
> >> +	struct bgx *bgx = context;
> >> +	struct acpi_device *adev;
> >> +	struct device *phy_dev;
> >> +	u32 phy_id;
> >> +
> >> +	if (acpi_bus_get_device(handle, &adev))
> >> +		goto out;
> >> +
> >> +	SET_NETDEV_DEV(&bgx->lmac[bgx->lmac_count].netdev, &bgx->pdev->dev);
> >> +
> >> +	acpi_get_mac_address(adev, bgx->lmac[bgx->lmac_count].mac);
> >> +
> >> +	bgx->lmac[bgx->lmac_count].lmacid = bgx->lmac_count;
> >> +
> >> +	if (acpi_dev_get_property_reference(adev, "phy-handle", 0, &args))
> >> +		goto out;
> >> +
> >> +	if (acpi_dev_get_property(args.adev, "phy-channel", ACPI_TYPE_INTEGER, &prop))
> >> +		goto out;
> >
> > Likewise for any inter-device properties, so that we can actually handle
> > them in a generic fashion, and avoid / learn from the mistakes we've
> > already handled with DT.
> 
> This is the fallacy of the ACPI is superior to DT argument.  The 
> specification of PHY topology and MAC addresses is well standardized in 
> DT, there is no question about what the proper way to specify it is. 
> Under ACPI, it is the Wild West, there is no specification, so each 
> system design is forced to invent something, and everybody comes up with 
> an incompatible implementation.

Indeed.

If ACPI is going to handle it, it should handle it properly. I really
don't see the point in bodging properties together in a less standard
manner than DT, especially for inter-device relationships.

Doing so is painful for _everyone_, and it's extremely unlikely that
other ACPI-aware OSs will actually support these custom descriptions,
making this Linux-specific, and breaking the rationale for using ACPI in
the first place -- a standard that says "just do non-standard stuff" is
not a usable standard.

For intra-device properties, we should standardise what we can, but
vendor-specific stuff is ok -- this can be self-contained within a
driver.

For inter-device relationships ACPI _must_ gain a better model of
componentised devices. It's simply unworkable otherwise, and as you
point out it's fallacious to say that because ACPI is being used that
something is magically industry standard, portable, etc.

This is not your problem in particular; the entire handling of _DSD so
far is a joke IMO.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] net, thunder, bgx: Add support for ACPI binding.

[Mark Rutland]

[Correcting the devicetree list address, which I typo'd in my original
reply]

[resending to _really_ correct the address, apologies for the spam]

> >> +static const char * const addr_propnames[] = {
> >> +	"mac-address",
> >> +	"local-mac-address",
> >> +	"address",
> >> +};
> >
> > If these are going to be generally necessary, then we should get them
> > adopted as standardised _DSD properties (ideally just one of them).
> 
> As far as I can tell, and please correct me if I am wrong, ACPI-6.0 
> doesn't contemplate MAC addresses.
> 
> Today we are using "mac-address", which is an Integer containing the MAC 
> address in its lowest order 48 bits in Little-Endian byte order.
> 
> The hardware and ACPI tables are here today, and we would like to 
> support it.  If some future ACPI specification specifies a standard way 
> to do this, we will probably adapt the code to do this in a standard manner.
> 
> 
> >
> > [...]
> >
> >> +static acpi_status bgx_acpi_register_phy(acpi_handle handle,
> >> +					 u32 lvl, void *context, void **rv)
> >> +{
> >> +	struct acpi_reference_args args;
> >> +	const union acpi_object *prop;
> >> +	struct bgx *bgx = context;
> >> +	struct acpi_device *adev;
> >> +	struct device *phy_dev;
> >> +	u32 phy_id;
> >> +
> >> +	if (acpi_bus_get_device(handle, &adev))
> >> +		goto out;
> >> +
> >> +	SET_NETDEV_DEV(&bgx->lmac[bgx->lmac_count].netdev, &bgx->pdev->dev);
> >> +
> >> +	acpi_get_mac_address(adev, bgx->lmac[bgx->lmac_count].mac);
> >> +
> >> +	bgx->lmac[bgx->lmac_count].lmacid = bgx->lmac_count;
> >> +
> >> +	if (acpi_dev_get_property_reference(adev, "phy-handle", 0, &args))
> >> +		goto out;
> >> +
> >> +	if (acpi_dev_get_property(args.adev, "phy-channel", ACPI_TYPE_INTEGER, &prop))
> >> +		goto out;
> >
> > Likewise for any inter-device properties, so that we can actually handle
> > them in a generic fashion, and avoid / learn from the mistakes we've
> > already handled with DT.
> 
> This is the fallacy of the ACPI is superior to DT argument.  The 
> specification of PHY topology and MAC addresses is well standardized in 
> DT, there is no question about what the proper way to specify it is. 
> Under ACPI, it is the Wild West, there is no specification, so each 
> system design is forced to invent something, and everybody comes up with 
> an incompatible implementation.

Indeed.

If ACPI is going to handle it, it should handle it properly. I really
don't see the point in bodging properties together in a less standard
manner than DT, especially for inter-device relationships.

Doing so is painful for _everyone_, and it's extremely unlikely that
other ACPI-aware OSs will actually support these custom descriptions,
making this Linux-specific, and breaking the rationale for using ACPI in
the first place -- a standard that says "just do non-standard stuff" is
not a usable standard.

For intra-device properties, we should standardise what we can, but
vendor-specific stuff is ok -- this can be self-contained within a
driver.

For inter-device relationships ACPI _must_ gain a better model of
componentised devices. It's simply unworkable otherwise, and as you
point out it's fallacious to say that because ACPI is being used that
something is magically industry standard, portable, etc.

This is not your problem in particular; the entire handling of _DSD so
far is a joke IMO.

Thanks,
Mark.

Re: [PATCH 2/2] net, thunder, bgx: Add support for ACPI binding.

[Rafael J. Wysocki]

Hi Mark,

On Fri, Aug 7, 2015 at 7:51 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> [Correcting the devicetree list address, which I typo'd in my original
> reply]
>
>> >> +static const char * const addr_propnames[] = {
>> >> +  "mac-address",
>> >> +  "local-mac-address",
>> >> +  "address",
>> >> +};
>> >
>> > If these are going to be generally necessary, then we should get them
>> > adopted as standardised _DSD properties (ideally just one of them).
>>
>> As far as I can tell, and please correct me if I am wrong, ACPI-6.0
>> doesn't contemplate MAC addresses.
>>
>> Today we are using "mac-address", which is an Integer containing the MAC
>> address in its lowest order 48 bits in Little-Endian byte order.
>>
>> The hardware and ACPI tables are here today, and we would like to
>> support it.  If some future ACPI specification specifies a standard way
>> to do this, we will probably adapt the code to do this in a standard manner.
>>
>>
>> >
>> > [...]
>> >
>> >> +static acpi_status bgx_acpi_register_phy(acpi_handle handle,
>> >> +                                   u32 lvl, void *context, void **rv)
>> >> +{
>> >> +  struct acpi_reference_args args;
>> >> +  const union acpi_object *prop;
>> >> +  struct bgx *bgx = context;
>> >> +  struct acpi_device *adev;
>> >> +  struct device *phy_dev;
>> >> +  u32 phy_id;
>> >> +
>> >> +  if (acpi_bus_get_device(handle, &adev))
>> >> +          goto out;
>> >> +
>> >> +  SET_NETDEV_DEV(&bgx->lmac[bgx->lmac_count].netdev, &bgx->pdev->dev);
>> >> +
>> >> +  acpi_get_mac_address(adev, bgx->lmac[bgx->lmac_count].mac);
>> >> +
>> >> +  bgx->lmac[bgx->lmac_count].lmacid = bgx->lmac_count;
>> >> +
>> >> +  if (acpi_dev_get_property_reference(adev, "phy-handle", 0, &args))
>> >> +          goto out;
>> >> +
>> >> +  if (acpi_dev_get_property(args.adev, "phy-channel", ACPI_TYPE_INTEGER, &prop))
>> >> +          goto out;
>> >
>> > Likewise for any inter-device properties, so that we can actually handle
>> > them in a generic fashion, and avoid / learn from the mistakes we've
>> > already handled with DT.
>>
>> This is the fallacy of the ACPI is superior to DT argument.  The
>> specification of PHY topology and MAC addresses is well standardized in
>> DT, there is no question about what the proper way to specify it is.
>> Under ACPI, it is the Wild West, there is no specification, so each
>> system design is forced to invent something, and everybody comes up with
>> an incompatible implementation.
>
> Indeed.
>
> If ACPI is going to handle it, it should handle it properly. I really
> don't see the point in bodging properties together in a less standard
> manner than DT, especially for inter-device relationships.
>
> Doing so is painful for _everyone_, and it's extremely unlikely that
> other ACPI-aware OSs will actually support these custom descriptions,
> making this Linux-specific, and breaking the rationale for using ACPI in
> the first place -- a standard that says "just do non-standard stuff" is
> not a usable standard.
>
> For intra-device properties, we should standardise what we can, but
> vendor-specific stuff is ok -- this can be self-contained within a
> driver.
>
> For inter-device relationships ACPI _must_ gain a better model of
> componentised devices. It's simply unworkable otherwise, and as you
> point out it's fallacious to say that because ACPI is being used that
> something is magically industry standard, portable, etc.
>
> This is not your problem in particular; the entire handling of _DSD so
> far is a joke IMO.

It is actually useful to people as far as I can say.

Also, if somebody is going to use properties with ACPI, why whould
they use a different set of properties with DT?

Wouldn't it be more reasonable to use the same set in both cases?

Thanks,
Rafael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] net, thunder, bgx: Add support for ACPI binding.

[David Daney]

On 08/07/2015 05:05 PM, Rafael J. Wysocki wrote:
> Hi Mark,
>
> On Fri, Aug 7, 2015 at 7:51 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> [Correcting the devicetree list address, which I typo'd in my original
>> reply]
>>
>>>>> +static const char * const addr_propnames[] = {
>>>>> +  "mac-address",
>>>>> +  "local-mac-address",
>>>>> +  "address",
>>>>> +};
>>>>
>>>> If these are going to be generally necessary, then we should get them
>>>> adopted as standardised _DSD properties (ideally just one of them).
>>>
>>> As far as I can tell, and please correct me if I am wrong, ACPI-6.0
>>> doesn't contemplate MAC addresses.
>>>
>>> Today we are using "mac-address", which is an Integer containing the MAC
>>> address in its lowest order 48 bits in Little-Endian byte order.
>>>
>>> The hardware and ACPI tables are here today, and we would like to
>>> support it.  If some future ACPI specification specifies a standard way
>>> to do this, we will probably adapt the code to do this in a standard manner.
>>>
>>>
>>>>
>>>> [...]
>>>>
>>>>> +static acpi_status bgx_acpi_register_phy(acpi_handle handle,
>>>>> +                                   u32 lvl, void *context, void **rv)
>>>>> +{
>>>>> +  struct acpi_reference_args args;
>>>>> +  const union acpi_object *prop;
>>>>> +  struct bgx *bgx = context;
>>>>> +  struct acpi_device *adev;
>>>>> +  struct device *phy_dev;
>>>>> +  u32 phy_id;
>>>>> +
>>>>> +  if (acpi_bus_get_device(handle, &adev))
>>>>> +          goto out;
>>>>> +
>>>>> +  SET_NETDEV_DEV(&bgx->lmac[bgx->lmac_count].netdev, &bgx->pdev->dev);
>>>>> +
>>>>> +  acpi_get_mac_address(adev, bgx->lmac[bgx->lmac_count].mac);
>>>>> +
>>>>> +  bgx->lmac[bgx->lmac_count].lmacid = bgx->lmac_count;
>>>>> +
>>>>> +  if (acpi_dev_get_property_reference(adev, "phy-handle", 0, &args))
>>>>> +          goto out;
>>>>> +
>>>>> +  if (acpi_dev_get_property(args.adev, "phy-channel", ACPI_TYPE_INTEGER, &prop))
>>>>> +          goto out;
>>>>
>>>> Likewise for any inter-device properties, so that we can actually handle
>>>> them in a generic fashion, and avoid / learn from the mistakes we've
>>>> already handled with DT.
>>>
>>> This is the fallacy of the ACPI is superior to DT argument.  The
>>> specification of PHY topology and MAC addresses is well standardized in
>>> DT, there is no question about what the proper way to specify it is.
>>> Under ACPI, it is the Wild West, there is no specification, so each
>>> system design is forced to invent something, and everybody comes up with
>>> an incompatible implementation.
>>
>> Indeed.
>>
>> If ACPI is going to handle it, it should handle it properly. I really
>> don't see the point in bodging properties together in a less standard
>> manner than DT, especially for inter-device relationships.
>>
>> Doing so is painful for _everyone_, and it's extremely unlikely that
>> other ACPI-aware OSs will actually support these custom descriptions,
>> making this Linux-specific, and breaking the rationale for using ACPI in
>> the first place -- a standard that says "just do non-standard stuff" is
>> not a usable standard.
>>
>> For intra-device properties, we should standardise what we can, but
>> vendor-specific stuff is ok -- this can be self-contained within a
>> driver.
>>
>> For inter-device relationships ACPI _must_ gain a better model of
>> componentised devices. It's simply unworkable otherwise, and as you
>> point out it's fallacious to say that because ACPI is being used that
>> something is magically industry standard, portable, etc.
>>
>> This is not your problem in particular; the entire handling of _DSD so
>> far is a joke IMO.
>
> It is actually useful to people as far as I can say.
>
> Also, if somebody is going to use properties with ACPI, why whould
> they use a different set of properties with DT?
>
> Wouldn't it be more reasonable to use the same set in both cases?

Yes, but there is still quite a bit of leeway to screw things up.


FWIW:  http://www.uefi.org/sites/default/files/resources/nic-request-v2.pdf

This actually seems to have been adopted by the UEFI people as a
"Standard", I am not sure where a record of this is kept though.

So, we are changing our firmware to use this standard (which is quite
similar the the DT with respect to MAC addresses).

Thanks,
David Daney

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] net, thunder, bgx: Add support for ACPI binding.

[Rafael J. Wysocki]

Hi David,

On Sat, Aug 8, 2015 at 2:11 AM, David Daney <ddaney@caviumnetworks.com> wrote:
> On 08/07/2015 05:05 PM, Rafael J. Wysocki wrote:

[cut]

>>
>> It is actually useful to people as far as I can say.
>>
>> Also, if somebody is going to use properties with ACPI, why whould
>> they use a different set of properties with DT?
>>
>> Wouldn't it be more reasonable to use the same set in both cases?
>
>
> Yes, but there is still quite a bit of leeway to screw things up.

That I have to agree with, unfortunately.

On the other hand, this is a fairly new concept and we need to gain
some experience with it to be able to come up with best practices and
so on.  Cases like yours are really helpful here.

> FWIW:  http://www.uefi.org/sites/default/files/resources/nic-request-v2.pdf
>
> This actually seems to have been adopted by the UEFI people as a
> "Standard", I am not sure where a record of this is kept though.

Work on this is in progress, but far from completion.  Essentially,
what's needed is more pressure from vendors who want to use properties
in their firmware.

> So, we are changing our firmware to use this standard (which is quite
> similar the the DT with respect to MAC addresses).

Cool. :-)

Thanks,
Rafael

_DSD standardization note (WAS: Re: [PATCH 2/2] net, thunder, bgx: Add support for ACPI binding.)

[Jon Masters]

Following up on this thread after finally seeing it...figured I would
send something just for the archive mainly (we discussed this in person
recently at a few different events and I think are aligned already).

On 08/07/2015 08:28 PM, Rafael J. Wysocki wrote:
> Hi David,
> 
> On Sat, Aug 8, 2015 at 2:11 AM, David Daney <ddaney@caviumnetworks.com> wrote:
>> On 08/07/2015 05:05 PM, Rafael J. Wysocki wrote:
> 
> [cut]
> 
>>>
>>> It is actually useful to people as far as I can say.
>>>
>>> Also, if somebody is going to use properties with ACPI, why whould
>>> they use a different set of properties with DT?

Generally speaking, if there's a net new thing to handle, there is of
course no particular problem with using DT as an inspiration, but we
need to be conscious of the fact that Linux isn't the only Operating
System that may need to support these bindings, so the correct thing (as
stated by many of you, and below, and in person also recently - so we
are aligned) is to get this (the MAC address binding for _DSD in ACPI)
standardized properly through UEFI where everyone who has a vest OS
interest beyond Linux can also have their own involvement as well. As
discussed, that doesn't make it part of ACPI6.0, just a binding.

FWIW I made a decision on the Red Hat end that in our guidelines to
partners for ARM RHEL(SA - Server for ARM) builds we would not generally
endorse any use of _DSD, with the exception of the MAC address binding
being discussed here. In that case, I realized I had not been fully
prescriptive enough with the vendors building early hw in that I should
have realized this would happen and have required that they do this the
right way. MAC IP should be more sophisticated such that it can handle
being reset even after the firmware has loaded its MAC address(es).
Platform flash storage separate from UEFI variable storage (which is
being abused to contain too much now that DXE drivers then load into the
ACPI tables prior to exiting Boot Services, etc.) should contain the
actual MAC address(es), as it is done on other arches, and it should not
be necessary to communicate this via ACPI tables to begin with (that's a
cost saving embedded concept that should not happen on server systems in
the general case). I have already had several unannounced future designs
adjusted in light of this _DSD usage.

In the case of providing MAC address information (only) by _DSD, I
connected the initial ARMv8 SoC silicon vendors who needed to use such a
hack to ensure they were using the same properties, and will followup
off list to ensure Cavium are looped into that. But, we do need to get
the _DSD property for MAC address provision standardized through UEFI
properly as an official binding rather than just a link on the website,
and then we need to be extremely careful not to grow any further
dependence upon _DSD elsewhere. Generally, if you're using that approach
on a server system (other than for this MAC case), your firmware or
design (or both) need to be modified to not use _DSD.

Jon.

Re: _DSD standardization note (WAS: Re: [PATCH 2/2] net, thunder, bgx: Add support for ACPI binding.)

[David Daney]

On 09/05/2015 01:00 PM, Jon Masters wrote:
> Following up on this thread after finally seeing it...figured I would
> send something just for the archive mainly (we discussed this in person
> recently at a few different events and I think are aligned already).
>
> On 08/07/2015 08:28 PM, Rafael J. Wysocki wrote:
>> Hi David,
>>
>> On Sat, Aug 8, 2015 at 2:11 AM, David Daney <ddaney@caviumnetworks.com> wrote:
>>> On 08/07/2015 05:05 PM, Rafael J. Wysocki wrote:
>>
>> [cut]
>>
>>>>
>>>> It is actually useful to people as far as I can say.
>>>>
>>>> Also, if somebody is going to use properties with ACPI, why whould
>>>> they use a different set of properties with DT?
>
> Generally speaking, if there's a net new thing to handle, there is of
> course no particular problem with using DT as an inspiration, but we
> need to be conscious of the fact that Linux isn't the only Operating
> System that may need to support these bindings, so the correct thing (as
> stated by many of you, and below, and in person also recently - so we
> are aligned) is to get this (the MAC address binding for _DSD in ACPI)
> standardized properly through UEFI where everyone who has a vest OS
> interest beyond Linux can also have their own involvement as well. As
> discussed, that doesn't make it part of ACPI6.0, just a binding.
>
> FWIW I made a decision on the Red Hat end that in our guidelines to
> partners for ARM RHEL(SA - Server for ARM) builds we would not generally
> endorse any use of _DSD, with the exception of the MAC address binding
> being discussed here. In that case, I realized I had not been fully
> prescriptive enough with the vendors building early hw in that I should
> have realized this would happen and have required that they do this the
> right way. MAC IP should be more sophisticated such that it can handle
> being reset even after the firmware has loaded its MAC address(es).
> Platform flash storage separate from UEFI variable storage (which is
> being abused to contain too much now that DXE drivers then load into the
> ACPI tables prior to exiting Boot Services, etc.) should contain the
> actual MAC address(es), as it is done on other arches, and it should not
> be necessary to communicate this via ACPI tables to begin with (that's a
> cost saving embedded concept that should not happen on server systems in
> the general case). I have already had several unannounced future designs
> adjusted in light of this _DSD usage.
>
> In the case of providing MAC address information (only) by _DSD, I
> connected the initial ARMv8 SoC silicon vendors who needed to use such a
> hack to ensure they were using the same properties, and will followup
> off list to ensure Cavium are looped into that. But, we do need to get
> the _DSD property for MAC address provision standardized through UEFI
> properly as an official binding rather than just a link on the website,
> and then we need to be extremely careful not to grow any further
> dependence upon _DSD elsewhere. Generally, if you're using that approach
> on a server system (other than for this MAC case), your firmware or
> design (or both) need to be modified to not use _DSD.

I think we need to be cognizant of the fact that ARMv8 SoCs do contain, 
and in the future will still contain, many different hardware bus 
interface devices.  These include I2C, MDIO, GPIO, xMII (x in {,SG,RGM, 
etc} network MAC interfaces).  In the context of network interfaces 
these are often used in conjunction with stand-alone PHY devices.

A network driver on such a system must know several things that cannot 
be probed, and thus must be communicated by the firmware:

  - PHY type/model-number.

  - PHY management channel (MDIO/I2C + management bus address)

  - PHY interrupt connection, if any, (Often a GPIO pin).

  - SFP eeprom location (Which I2C bus is it on).

On x86 systems, all those things were placed on a PCI NIC, and the 
configuration could be identified in a stand alone manner by the NIC 
driver, so everything was simple.

For SoC based systems, I don't see a better alternative than to expose 
the topology via firmware tables.  In the case of OF device-tree, this 
is done in a standard manner with "phy-handle" and "interrupts" 
properties utilizing phandle links to traverse branches of the device tree.

I am not an ACPI guru, so I don't know for certain the best way to 
express this in ACPI, but it seems like _DSD may have to be involved.

David Daney