| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <9a98d026-7f70-a69b-64de-c77419888e42@amd.com>
Date: Tue, 13 Sep 2022 21:57:04 +0000
From: "Larson, Bradley" <Bradley.Larson@....com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
"Larson, Bradley" <Bradley.Larson@....com>,
Rob Herring <robh@...nel.org>
CC: "linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
"adrian.hunter@...el.com" <adrian.hunter@...el.com>,
"alcooperx@...il.com" <alcooperx@...il.com>,
"andy.shevchenko@...il.com" <andy.shevchenko@...il.com>,
"arnd@...db.de" <arnd@...db.de>,
"brijeshkumar.singh@....com" <brijeshkumar.singh@....com>,
"catalin.marinas@....com" <catalin.marinas@....com>,
"gsomlo@...il.com" <gsomlo@...il.com>,
"gerg@...ux-m68k.org" <gerg@...ux-m68k.org>,
"krzysztof.kozlowski+dt@...aro.org"
<krzysztof.kozlowski+dt@...aro.org>,
"lee.jones@...aro.org" <lee.jones@...aro.org>,
"broonie@...nel.org" <broonie@...nel.org>,
"yamada.masahiro@...ionext.com" <yamada.masahiro@...ionext.com>,
"p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
"piotrs@...ence.com" <piotrs@...ence.com>,
"p.yadav@...com" <p.yadav@...com>,
"rdunlap@...radead.org" <rdunlap@...radead.org>,
"samuel@...lland.org" <samuel@...lland.org>,
"fancer.lancer@...il.com" <fancer.lancer@...il.com>,
"Suthikulpanit, Suravee" <Suravee.Suthikulpanit@....com>,
"Lendacky, Thomas" <Thomas.Lendacky@....com>,
"ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
"will@...nel.org" <will@...nel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH v6 06/17] dt-bindings: mfd: amd,pensando-elbasr: Add AMD
Pensando Elba System Resource chip
On 9/8/22 4:27 AM, Krzysztof Kozlowski wrote:
> On 01/09/2022 22:37, Larson, Bradley wrote:
>> On 9/1/22 12:20 AM, Krzysztof Kozlowski wrote:
>>> On 01/09/2022 02:01, Larson, Bradley wrote:
>>>>>> + is implemented by a sub-device reset-controller which accesses
>>>>>> + a CS0 control register.
>>>>>> +
>>>>>> +maintainers:
>>>>>> + - Brad Larson <blarson@....com>
>>>>>> +
>>>>>> +properties:
>>>>>> + compatible:
>>>>>> + items:
>>>>>> + - enum:
>>>>>> + - amd,pensando-elbasr
>>>>>> +
>>>>>> + spi-max-frequency:
>>>>>> + description: Maximum SPI frequency of the device in Hz.
>>>>> No need for generic descriptions of common properties.
>>>> Changed to "spi-max-frequency: true" and moved to end of properties.
>>> Then you should rather reference spi-peripheral-props just like other
>>> SPI devices.
>>
>> Will look at this dependent on the result of below
>>
>>
>>>>>> +
>>>>>> + reg:
>>>>>> + maxItems: 1
>>>>>> +
>>>>>> + '#address-cells':
>>>>>> + const: 1
>>>>>> +
>>>>>> + '#size-cells':
>>>>>> + const: 0
>>>>>> +
>>>>>> + interrupts:
>>>>>> + maxItems: 1
>>>>>> +
>>>>>> +required:
>>>>>> + - compatible
>>>>>> + - reg
>>>>>> + - spi-max-frequency
>>>>>> +
>>>>>> +patternProperties:
>>>>>> + '^reset-controller@[a-f0-9]+$':
>>>>>> + $ref: /schemas/reset/amd,pensando-elbasr-reset.yaml
>>>>>> +
>>>>>> +additionalProperties: false
>>>>>> +
>>>>>> +examples:
>>>>>> + - |
>>>>>> + #include <dt-bindings/interrupt-controller/arm-gic.h>
>>>>>> +
>>>>>> + spi {
>>>>>> + #address-cells = <1>;
>>>>>> + #size-cells = <0>;
>>>>>> + num-cs = <4>;
>>>>>> +
>>>>>> + sysc: system-controller@0 {
>>>>>> + compatible = "amd,pensando-elbasr";
>>>>>> + reg = <0>;
>>>>>> + #address-cells = <1>;
>>>>>> + #size-cells = <0>;
>>>>>> + spi-max-frequency = <12000000>;
>>>>>> +
>>>>>> + rstc: reset-controller@0 {
>>>>>> + compatible = "amd,pensando-elbasr-reset";
>>>>>> + reg = <0>;
>>>>> What does 0 represent here? A register address within 'elbasr' device?
>>>> Removed, I recall a check threw a warning or error without reg.
>>>>
>>>>> Why do you need a child node for this? Are there other sub-devices and
>>>>> your binding is incomplete? Just put '#reset-cells' in the parent.
>>>> Without a reset-controller node and booting the function
>>>> __of_reset_control_get(...) fails to find a match in the list here
>>> That's not actually the answer to the question. There was no concerns
>>> whether you need or not reset controller. The question was why do you
>>> need a child device instead of elbasr being the reset controller.
>>>
>>> Your answer does not cover this at all, so again - why do you need a
>>> child for this?
>>>
>> If the parent becomes a reset-controller compatible with
>> "amd,pensando-elbasr-reset" then the /dev node will not be created
> Why /dev node will not be created? How bindings affect having or not
> having something in /dev ?
>
>> as there is no match to "amd,pensando-elbasr" for cs0. For cs0 internal
>> to linux the reset-controller manages one register bit to hardware reset
>> the mmc device. A userspace application opens the device node to manage
>> transceiver leds, system leds, reporting temps to host, other reset
>> controls, etc. Looking at future requirements there likely will be other
>> child devices.
> You mean "amd,pensando-elbasr" will instantiate some more devices? Why
> you cannot add the binding for them now? This is actually important
> because earlier we agreed you remove unit address from children.
>
>> Going down this path with one compatible should reset-elbasr.c just be
>> deleted and fold it into the parent driver pensando-elbasr.c? Then I
>> wonder if it even belongs in drivers/mfd and should just be modified
>> and put in drivers/spi.
> How is it related to bindings?
The compatible "amd,pensando-elbasr" is matched in
drivers/mfd/pensando-elbasr.c
which creates /dev/pensr0.<cs> for spi chip-selects defined in the
parent node as:
num-cs = <4>;
cs-gpios = <0>, <0>, <&porta 1 GPIO_ACTIVE_LOW>,
<&porta 7 GPIO_ACTIVE_LOW>;
The compatible "amd,pensando-elbasr-reset" is in
drivers/reset/reset-elbasr.c
which uses regmap to control one bit in the function at cs0 to hardware
reset the eMMC.
This is the reason for the reset-controller child and the two driver
files. The
probe in drivers/mfd/pensando-elbasr.c is called 4 times, once per spi
chip-select
and for cs0 mfd_add_devices() is called for the reset controller.
I'll change "rstc: reset-controller@0" to "rstc: reset-controller".
Maybe I've gotten off track following what looked like an appropriate model
to follow ("altr,a10sr") that was initially added in 2017 and has the same
device topology which is SoC <= spi => CPLD/FPGA where a10sr has a 3rd
driver
file for a gpio controller resulting in two child nodes.
In our case its not one function its four in the device where the function
at chip-select 0 is where the hardware team provided the bit to control
eMMC hardware reset. Is this a bad approach to follow and if so please
recommend an acceptable approach. Also, "amd,pensando-elbasr" will not
instantiate more devices.
Snippets below for a10sr in linux-next: Device on other end of spi,
one chip select, two child devices and 3 driver files in mfd, reset, and
gpio.
FILE: arch/arm/boot/dts/socfpga_arria10_socdk.dtsi:
&spi1 {
status = "okay";
resource-manager@0 {
compatible = "altr,a10sr";
reg = <0>;
spi-max-frequency = <100000>;
/* low-level active IRQ at GPIO1_5 */
interrupt-parent = <&portb>;
interrupts = <5 IRQ_TYPE_LEVEL_LOW>;
interrupt-controller;
#interrupt-cells = <2>;
a10sr_gpio: gpio-controller {
compatible = "altr,a10sr-gpio";
gpio-controller;
#gpio-cells = <2>;
};
a10sr_rst: reset-controller {
compatible = "altr,a10sr-reset";
#reset-cells = <1>;
};
};
};
FILE: drivers/mfd/altera-a10sr.c (parent)
static const struct mfd_cell altr_a10sr_subdev_info[] = {
{
.name = "altr_a10sr_gpio",
.of_compatible = "altr,a10sr-gpio",
},
{
.name = "altr_a10sr_reset",
.of_compatible = "altr,a10sr-reset",
},
};
static const struct of_device_id altr_a10sr_spi_of_match[] = {
{ .compatible = "altr,a10sr" },
{ },
};
FILE: drivers/reset/reset-a10sr.c (reset driver)
static const struct of_device_id a10sr_reset_of_match[] = {
{ .compatible = "altr,a10sr-reset" },
{ },
};
FILE: ./drivers/gpio/gpio-altera-a10sr.c (gpio driver)
static const struct of_device_id altr_a10sr_gpio_of_match[] = {
{ .compatible = "altr,a10sr-gpio" },
{ },
};
In comparision, the pensando device is also on the other end of spi,
four chip selects with /dev created for each for userspace control,
and one child device on cs0 for hw reset emmc that the Linux block
layer controls (single bit managed only by kernel).
Pensando:
&spi0 {
num-cs = <4>;
cs-gpios = <0>, <0>, <&porta 1 GPIO_ACTIVE_LOW>,
<&porta 7 GPIO_ACTIVE_LOW>;
status = "okay";
system-controller@0 {
compatible = "amd,pensando-elbasr";
reg = <0>;
#address-cells = <1>;
#size-cells = <0>;
spi-max-frequency = <12000000>;
rstc: reset-controller {
compatible = "amd,pensando-elbasr-reset";
#reset-cells = <1>;
};
};
system-controller@1 {
compatible = "amd,pensando-elbasr";
reg = <1>;
spi-max-frequency = <12000000>;
};
system-controller@2 {
compatible = "amd,pensando-elbasr";
reg = <2>;
spi-max-frequency = <12000000>;
interrupt-parent = <&porta>;
interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
};
system-controller@3 {
compatible = "amd,pensando-elbasr";
reg = <3>;
spi-max-frequency = <12000000>;
};
};
Regards,
Brad
Powered by blists - more mailing lists