| 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: <7421647b-ae85-4f34-843c-02f1fb21d7f3@redhat.com>
Date: Thu, 22 May 2025 10:54:23 +0200
From: Ivan Vecera <ivecera@...hat.com>
To: Lee Jones <lee@...nel.org>
Cc: Andy Shevchenko <andy.shevchenko@...il.com>, netdev@...r.kernel.org,
Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>,
Jiri Pirko <jiri@...nulli.us>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Prathosh Satish <Prathosh.Satish@...rochip.com>,
"David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
Michal Schmidt <mschmidt@...hat.com>, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH net-next v7 8/8] mfd: zl3073x: Register DPLL sub-device
during init
On 22. 05. 25 9:39 dop., Lee Jones wrote:
> On Tue, 13 May 2025, Ivan Vecera wrote:
>
>> On 13. 05. 25 11:41 dop., Lee Jones wrote:
>>> On Mon, 12 May 2025, Ivan Vecera wrote:
>>>
>>>> On 07. 05. 25 5:26 odp., Lee Jones wrote:
>>>>> On Wed, 07 May 2025, Andy Shevchenko wrote:
>>>>>
>>>>>> On Wed, May 07, 2025 at 03:56:37PM +0200, Ivan Vecera wrote:
>>>>>>> On 07. 05. 25 3:41 odp., Andy Shevchenko wrote:
>>>>>>>> On Wed, May 7, 2025 at 3:45 PM Ivan Vecera <ivecera@...hat.com> wrote:
>>>>>>
>>>>>> ...
>>>>>>
>>>>>>>>> +static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
>>>>>>>>> + { .channel = 0, },
>>>>>>>>> + { .channel = 1, },
>>>>>>>>> + { .channel = 2, },
>>>>>>>>> + { .channel = 3, },
>>>>>>>>> + { .channel = 4, },
>>>>>>>>> +};
>>>>>>>>
>>>>>>>>> +static const struct mfd_cell zl3073x_devs[] = {
>>>>>>>>> + ZL3073X_CELL("zl3073x-dpll", 0),
>>>>>>>>> + ZL3073X_CELL("zl3073x-dpll", 1),
>>>>>>>>> + ZL3073X_CELL("zl3073x-dpll", 2),
>>>>>>>>> + ZL3073X_CELL("zl3073x-dpll", 3),
>>>>>>>>> + ZL3073X_CELL("zl3073x-dpll", 4),
>>>>>>>>> +};
>>>>>>>>
>>>>>>>>> +#define ZL3073X_MAX_CHANNELS 5
>>>>>>>>
>>>>>>>> Btw, wouldn't be better to keep the above lists synchronised like
>>>>>>>>
>>>>>>>> 1. Make ZL3073X_CELL() to use indexed variant
>>>>>>>>
>>>>>>>> [idx] = ...
>>>>>>>>
>>>>>>>> 2. Define the channel numbers
>>>>>>>>
>>>>>>>> and use them in both data structures.
>>>>>>>>
>>>>>>>> ...
>>>>>>>
>>>>>>> WDYM?
>>>>>>>
>>>>>>>> OTOH, I'm not sure why we even need this. If this is going to be
>>>>>>>> sequential, can't we make a core to decide which cell will be given
>>>>>>>> which id?
>>>>>>>
>>>>>>> Just a note that after introduction of PHC sub-driver the array will look
>>>>>>> like:
>>>>>>> static const struct mfd_cell zl3073x_devs[] = {
>>>>>>> ZL3073X_CELL("zl3073x-dpll", 0), // DPLL sub-dev for chan 0
>>>>>>> ZL3073X_CELL("zl3073x-phc", 0), // PHC sub-dev for chan 0
>>>>>>> ZL3073X_CELL("zl3073x-dpll", 1), // ...
>>>>>>> ZL3073X_CELL("zl3073x-phc", 1),
>>>>>>> ZL3073X_CELL("zl3073x-dpll", 2),
>>>>>>> ZL3073X_CELL("zl3073x-phc", 2),
>>>>>>> ZL3073X_CELL("zl3073x-dpll", 3),
>>>>>>> ZL3073X_CELL("zl3073x-phc", 3),
>>>>>>> ZL3073X_CELL("zl3073x-dpll", 4),
>>>>>>> ZL3073X_CELL("zl3073x-phc", 4), // PHC sub-dev for chan 4
>>>>>>> };
>>>>>>
>>>>>> Ah, this is very important piece. Then I mean only this kind of change
>>>>>>
>>>>>> enum {
>>>>>> // this or whatever meaningful names
>>>>>> ..._CH_0 0
>>>>>> ..._CH_1 1
>>>>>> ...
>>>>>> };
>>>>>>
>>>>>> static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
>>>>>> { .channel = ..._CH_0, },
>>>>>> ...
>>>>>> };
>>>>>>
>>>>>> static const struct mfd_cell zl3073x_devs[] = {
>>>>>> ZL3073X_CELL("zl3073x-dpll", ..._CH_0),
>>>>>> ZL3073X_CELL("zl3073x-phc", ..._CH_0),
>>>>>> ...
>>>>>> };
>>>>>
>>>>> This is getting hectic. All for a sequential enumeration. Seeing as
>>>>> there are no other differentiations, why not use IDA in the child
>>>>> instead?
>>>>
>>>> For that, there have to be two IDAs, one for DPLLs and one for PHCs...
>>>
>>> Sorry, can you explain a bit more. Why is this a problem?
>>>
>>> The IDA API is very simple.
>>>
>>> Much better than building your own bespoke MACROs.
>>
>> I will try to explain this in more detail... This MFD driver handles
>> chip family ZL3073x where the x == number of DPLL channels and can
>> be from <1, 5>.
>>
>> The driver creates 'x' DPLL sub-devices during probe and has to pass
>> channel number that should this sub-device use. Here can be used IDA
>> in DPLL sub-driver:
>> e.g. ida_alloc_max(zldev->channels, zldev->max_channels, GFP_KERNEL);
>>
>> This way the DPLL sub-device get its own unique channel ID to use.
>>
>> The situation is getting more complicated with PHC sub-devices because
>> the chip can provide UP TO 'x' PHC sub-devices depending on HW
>> configuration. To handle this the MFD driver has to check this HW config
>> for particular channel if it is capable to provide PHC functionality.
>>
>> E.g. ZL30735 chip has 5 channels, in this case the MFD driver should
>> create 5 DPLL sub-devices. And then lets say channel 0, 2 and 4 are
>> PHC capable. Then the MFD driver should create 3 PHC sub-devices and
>> pass 0, 2 resp. 4 for them.
>
> Where is the code that determines which channels are PHC capable?
It is not included in this series and will be added once the PTP driver
will be added. But the code looks like:
for (i = 0; i < ZL3073X_MAX_CHANNELS; i++) {
if (channel_is_in_nco_mode(..., i)) {
struct mfd_cell phc_dev = ZL3073X_CELL("zl3073x-phc", i);
rc = devm_mfd_add_devices(zldev->dev,
PLATFORM_DEVID_AUTO, &phc_dev,
1, NULL, 0, NULL);
...
}
}
Thanks,
Ivan
Powered by blists - more mailing lists