Re: [PATCH RFC 0/3] amba: bus: Move reading periphid operation from amba_match() to amba_probe()

[Zijun Hu]

On 2024/9/11 00:27, Saravana Kannan wrote:
> On Tue, Sep 10, 2024 at 5:17 AM Zijun Hu <zijun_hu@xxxxxxxxxx> wrote:
>>
>> On 2024/9/9 15:24, Saravana Kannan wrote:
>>> On Sun, Sep 8, 2024 at 4:38 PM Zijun Hu <zijun_hu@xxxxxxxxxx> wrote:
>>>>
>>>> This patch series is to make amba_match(), as bus_type @amba_bustype's
>>>> match(), also follow below ideal rule:
>>>>
>>> Also, patch 3/3 is not at all easy to understand and seems to be doing
>>> way more than what the commit message is trying to do.
>>>
>>
>> thanks for your code review.
>>
>> let me explain the issue here firstly to go on with discussion, will
>> correct it by next revision.
>>
>> amba_match(), as bus_type @amba_bustype's match(), operate hardware to
>> read id, may return -EPROBE_DEFER consequently.
>>
>> this design is not very good and has several disadvantages shown below:
>>
>> 1) it is not good time to operate hardware in a bus_type's match().
>>    hardware is not ready to operate normally in a bus_type's match()
>>    as driver_probe_device() shown, there are still many preparations
>>    to make hardware to operate after a bus_type's match(), for example,
>>    resuming device and its ancestors, ensuring all its suppliers have
>>    drivers bound, activating its power domain, ...
>>
.....

>> 5) amba_match() is the only bus_type's match which breaks below ideal
>> rule in current kernel tree:
>>    bus_type's match() should only return bool type compatible integer 0
>> or 1 ideally since its main operations are lookup and comparison normally.
> 
> All of this used to happen even if the bus match wasn't doing what
> it's doing today. You don't seem to have full context on how amba
> devices are added and probed. What you see now is a clean
> up/simplification of how things used to work.
> 
> Please go read this patch history and git log history for these files
> to get more context.
> 
> Nack for the entire series. It'll never go in.
> 

sorry, not agree with you.

IMO, it is easy to make amba_match() return bool type as shown below:

make amba_match() always match with AMBA device with INvalid periphid
and move reading id operation into amba_dma_configure().

Above solution can have the same logical as current one but it looks ugly.

so i make below optimizations to get this patch series:

1) only make AMBA device with INvalid periphid match with existing empty
   amba_proxy_drv to reduce unnecessary reading id operation.

2) moving reading id operation to amba_probe() looks more graceful.


Look at below 3 consecutive history commits:

git log --pretty='%h (\"%s\")' 656b8035b0ee -3
Commit: 656b8035b0ee ("ARM: 8524/1: driver cohandle -EPROBE_DEFER from
bus_type.match()")
Commit: 17f29d36e473 ("ARM: 8523/1: sa1111: ensure no negative value
gets returned on positive match")
Commit: 82ec2ba2b181 ("ARM: 8522/1: drivers: nvdimm: ensure no negative
value gets returned on positive match")

the first AMBA related commit breaks that a bus_type's match() have bool
type return value.
the remaining two commits at the same time really do not like negative
return value for a bus_type's match().

thanks
> -Saravana
> 
>>
>>
>> Our purpose is to solve this issue then enforce the ideal rule mentioned
>> in 5).
>>
>> so we send this patch series to start a topic about how to solve this
>> issue (^^).
>>
>>> -Saravana
>>>
>>
>>