diff mbox series

[RFC,1/2] spmi: Add support for multi-master

Message ID 20240207-spmi-multi-master-support-v1-1-ce57f301c7fd@linaro.org
State New
Headers
Series spmi: Add multi master support |

Commit Message

Abel Vesa Feb. 6, 2024, 11:33 p.m. UTC 
  Some newer SPMI controllers support multiple bus masters.
Such a master can control multiple slave devices. The generic
framework needs to be able to pass on the master id to the
controller-specific driver. So do that. The framework will
check if the devicetree child nodes are actually bus masters
and will register the devices for each master. The legacy
approach will still be supported for backwards compatibility.

Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
 drivers/spmi/spmi-mtk-pmif.c |  6 ++--
 drivers/spmi/spmi-pmic-arb.c | 10 +++---
 drivers/spmi/spmi.c          | 76 ++++++++++++++++++++++++++++++--------------
 include/linux/spmi.h         | 10 +++---
 4 files changed, 67 insertions(+), 35 deletions(-)

Comments

Dmitry Baryshkov Feb. 6, 2024, 11:55 p.m. UTC | #1 
On Wed, 7 Feb 2024 at 01:34, Abel Vesa <abel.vesa@linaro.org> wrote:
>
> Some newer SPMI controllers support multiple bus masters.
> Such a master can control multiple slave devices. The generic
> framework needs to be able to pass on the master id to the
> controller-specific driver. So do that. The framework will
> check if the devicetree child nodes are actually bus masters
> and will register the devices for each master. The legacy
> approach will still be supported for backwards compatibility.

Please remind me, are those two actual bus musters driving a single
bus in parallel or two SPMI buses being handled by a single device? In
the latter case this implementation is incorrect. There should be
multiple spmi_controller instances, one for each bus. Allocate them in
a loop and set ctrl->dev.of_node after allocating.

>
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
>  drivers/spmi/spmi-mtk-pmif.c |  6 ++--
>  drivers/spmi/spmi-pmic-arb.c | 10 +++---
>  drivers/spmi/spmi.c          | 76 ++++++++++++++++++++++++++++++--------------
>  include/linux/spmi.h         | 10 +++---
>  4 files changed, 67 insertions(+), 35 deletions(-)
Abel Vesa Feb. 7, 2024, 7:19 a.m. UTC | #2 
On 24-02-07 01:55:39, Dmitry Baryshkov wrote:
> On Wed, 7 Feb 2024 at 01:34, Abel Vesa <abel.vesa@linaro.org> wrote:
> >
> > Some newer SPMI controllers support multiple bus masters.
> > Such a master can control multiple slave devices. The generic
> > framework needs to be able to pass on the master id to the
> > controller-specific driver. So do that. The framework will
> > check if the devicetree child nodes are actually bus masters
> > and will register the devices for each master. The legacy
> > approach will still be supported for backwards compatibility.
> 
> Please remind me, are those two actual bus musters driving a single
> bus in parallel or two SPMI buses being handled by a single device? In
> the latter case this implementation is incorrect. There should be
> multiple spmi_controller instances, one for each bus. Allocate them in
> a loop and set ctrl->dev.of_node after allocating.

It's two SPMI buses (two sets of wires) handled by the same controller,
HW-wise.

If we register two spmi controllers with the kernel framework, it will
be HW inaccurate, because there is just one controller which has
multiple masters.

I'm not saying it might not work. But, to me, it looks more like a hack.

Basically, we would be mapping HW bus masters to kernel controllers.

> 
> >
> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > ---
> >  drivers/spmi/spmi-mtk-pmif.c |  6 ++--
> >  drivers/spmi/spmi-pmic-arb.c | 10 +++---
> >  drivers/spmi/spmi.c          | 76 ++++++++++++++++++++++++++++++--------------
> >  include/linux/spmi.h         | 10 +++---
> >  4 files changed, 67 insertions(+), 35 deletions(-)
> 
> -- 
> With best wishes
> Dmitry
Dmitry Baryshkov Feb. 7, 2024, 7:23 a.m. UTC | #3 
On Wed, 7 Feb 2024 at 09:19, Abel Vesa <abel.vesa@linaro.org> wrote:
>
> On 24-02-07 01:55:39, Dmitry Baryshkov wrote:
> > On Wed, 7 Feb 2024 at 01:34, Abel Vesa <abel.vesa@linaro.org> wrote:
> > >
> > > Some newer SPMI controllers support multiple bus masters.
> > > Such a master can control multiple slave devices. The generic
> > > framework needs to be able to pass on the master id to the
> > > controller-specific driver. So do that. The framework will
> > > check if the devicetree child nodes are actually bus masters
> > > and will register the devices for each master. The legacy
> > > approach will still be supported for backwards compatibility.
> >
> > Please remind me, are those two actual bus musters driving a single
> > bus in parallel or two SPMI buses being handled by a single device? In
> > the latter case this implementation is incorrect. There should be
> > multiple spmi_controller instances, one for each bus. Allocate them in
> > a loop and set ctrl->dev.of_node after allocating.
>
> It's two SPMI buses (two sets of wires) handled by the same controller,
> HW-wise.
>
> If we register two spmi controllers with the kernel framework, it will
> be HW inaccurate, because there is just one controller which has
> multiple masters.

struct spmi_controller is a controller for a single bus. Inside your
device you have two SPMI buses, each can be controlled by its own
struct spmi_controller. Just like devices that control multiple I2C,
SPI or USB busses register a separate instance of the bus controller.

>
> I'm not saying it might not work. But, to me, it looks more like a hack.
>
> Basically, we would be mapping HW bus masters to kernel controllers.

Buses, not just masters.

>
> >
> > >
> > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > > ---
> > >  drivers/spmi/spmi-mtk-pmif.c |  6 ++--
> > >  drivers/spmi/spmi-pmic-arb.c | 10 +++---
> > >  drivers/spmi/spmi.c          | 76 ++++++++++++++++++++++++++++++--------------
> > >  include/linux/spmi.h         | 10 +++---
> > >  4 files changed, 67 insertions(+), 35 deletions(-)
> >
> > --
> > With best wishes
> > Dmitry
Abel Vesa Feb. 7, 2024, 9:08 a.m. UTC | #4 
On 24-02-07 09:23:09, Dmitry Baryshkov wrote:
> On Wed, 7 Feb 2024 at 09:19, Abel Vesa <abel.vesa@linaro.org> wrote:
> >
> > On 24-02-07 01:55:39, Dmitry Baryshkov wrote:
> > > On Wed, 7 Feb 2024 at 01:34, Abel Vesa <abel.vesa@linaro.org> wrote:
> > > >
> > > > Some newer SPMI controllers support multiple bus masters.
> > > > Such a master can control multiple slave devices. The generic
> > > > framework needs to be able to pass on the master id to the
> > > > controller-specific driver. So do that. The framework will
> > > > check if the devicetree child nodes are actually bus masters
> > > > and will register the devices for each master. The legacy
> > > > approach will still be supported for backwards compatibility.
> > >
> > > Please remind me, are those two actual bus musters driving a single
> > > bus in parallel or two SPMI buses being handled by a single device? In
> > > the latter case this implementation is incorrect. There should be
> > > multiple spmi_controller instances, one for each bus. Allocate them in
> > > a loop and set ctrl->dev.of_node after allocating.
> >
> > It's two SPMI buses (two sets of wires) handled by the same controller,
> > HW-wise.
> >
> > If we register two spmi controllers with the kernel framework, it will
> > be HW inaccurate, because there is just one controller which has
> > multiple masters.
> 
> struct spmi_controller is a controller for a single bus. Inside your
> device you have two SPMI buses, each can be controlled by its own
> struct spmi_controller. Just like devices that control multiple I2C,
> SPI or USB busses register a separate instance of the bus controller.

Well, this is what this patchset is trying to do in the generic part.
The SPMI controller supports multiple buses (HW-wise) and therefore SW
implementation shouldn't be tied to single bus requirement.

> 
> >
> > I'm not saying it might not work. But, to me, it looks more like a hack.
> >
> > Basically, we would be mapping HW bus masters to kernel controllers.
> 
> Buses, not just masters.
> 
> >
> > >
> > > >
> > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > > > ---
> > > >  drivers/spmi/spmi-mtk-pmif.c |  6 ++--
> > > >  drivers/spmi/spmi-pmic-arb.c | 10 +++---
> > > >  drivers/spmi/spmi.c          | 76 ++++++++++++++++++++++++++++++--------------
> > > >  include/linux/spmi.h         | 10 +++---
> > > >  4 files changed, 67 insertions(+), 35 deletions(-)
> > >
> > > --
> > > With best wishes
> > > Dmitry
> 
> 
> 
> -- 
> With best wishes
> Dmitry
Dmitry Baryshkov Feb. 7, 2024, 9:44 a.m. UTC | #5 
On Wed, 7 Feb 2024 at 11:08, Abel Vesa <abel.vesa@linaro.org> wrote:
>
> On 24-02-07 09:23:09, Dmitry Baryshkov wrote:
> > On Wed, 7 Feb 2024 at 09:19, Abel Vesa <abel.vesa@linaro.org> wrote:
> > >
> > > On 24-02-07 01:55:39, Dmitry Baryshkov wrote:
> > > > On Wed, 7 Feb 2024 at 01:34, Abel Vesa <abel.vesa@linaro.org> wrote:
> > > > >
> > > > > Some newer SPMI controllers support multiple bus masters.
> > > > > Such a master can control multiple slave devices. The generic
> > > > > framework needs to be able to pass on the master id to the
> > > > > controller-specific driver. So do that. The framework will
> > > > > check if the devicetree child nodes are actually bus masters
> > > > > and will register the devices for each master. The legacy
> > > > > approach will still be supported for backwards compatibility.
> > > >
> > > > Please remind me, are those two actual bus musters driving a single
> > > > bus in parallel or two SPMI buses being handled by a single device? In
> > > > the latter case this implementation is incorrect. There should be
> > > > multiple spmi_controller instances, one for each bus. Allocate them in
> > > > a loop and set ctrl->dev.of_node after allocating.
> > >
> > > It's two SPMI buses (two sets of wires) handled by the same controller,
> > > HW-wise.
> > >
> > > If we register two spmi controllers with the kernel framework, it will
> > > be HW inaccurate, because there is just one controller which has
> > > multiple masters.
> >
> > struct spmi_controller is a controller for a single bus. Inside your
> > device you have two SPMI buses, each can be controlled by its own
> > struct spmi_controller. Just like devices that control multiple I2C,
> > SPI or USB busses register a separate instance of the bus controller.
>
> Well, this is what this patchset is trying to do in the generic part.
> The SPMI controller supports multiple buses (HW-wise) and therefore SW
> implementation shouldn't be tied to single bus requirement.

SPMI controller doesn't support multiple buses. You have a device
which has two SPMI controllers inside. Just like all multi-bus
controllers.

>
> >
> > >
> > > I'm not saying it might not work. But, to me, it looks more like a hack.
> > >
> > > Basically, we would be mapping HW bus masters to kernel controllers.
> >
> > Buses, not just masters.
> >
> > >
> > > >
> > > > >
> > > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > > > > ---
> > > > >  drivers/spmi/spmi-mtk-pmif.c |  6 ++--
> > > > >  drivers/spmi/spmi-pmic-arb.c | 10 +++---
> > > > >  drivers/spmi/spmi.c          | 76 ++++++++++++++++++++++++++++++--------------
> > > > >  include/linux/spmi.h         | 10 +++---
> > > > >  4 files changed, 67 insertions(+), 35 deletions(-)
> > > >
> > > > --
> > > > With best wishes
> > > > Dmitry
> >
> >
> >
> > --
> > With best wishes
> > Dmitry
Dmitry Baryshkov Feb. 7, 2024, 11:45 a.m. UTC | #6 
On Wed, 7 Feb 2024 at 11:08, Abel Vesa <abel.vesa@linaro.org> wrote:
>
> On 24-02-07 09:23:09, Dmitry Baryshkov wrote:
> > On Wed, 7 Feb 2024 at 09:19, Abel Vesa <abel.vesa@linaro.org> wrote:
> > >
> > > On 24-02-07 01:55:39, Dmitry Baryshkov wrote:
> > > > On Wed, 7 Feb 2024 at 01:34, Abel Vesa <abel.vesa@linaro.org> wrote:
> > > > >
> > > > > Some newer SPMI controllers support multiple bus masters.
> > > > > Such a master can control multiple slave devices. The generic
> > > > > framework needs to be able to pass on the master id to the
> > > > > controller-specific driver. So do that. The framework will
> > > > > check if the devicetree child nodes are actually bus masters
> > > > > and will register the devices for each master. The legacy
> > > > > approach will still be supported for backwards compatibility.
> > > >
> > > > Please remind me, are those two actual bus musters driving a single
> > > > bus in parallel or two SPMI buses being handled by a single device? In
> > > > the latter case this implementation is incorrect. There should be
> > > > multiple spmi_controller instances, one for each bus. Allocate them in
> > > > a loop and set ctrl->dev.of_node after allocating.
> > >
> > > It's two SPMI buses (two sets of wires) handled by the same controller,
> > > HW-wise.
> > >
> > > If we register two spmi controllers with the kernel framework, it will
> > > be HW inaccurate, because there is just one controller which has
> > > multiple masters.
> >
> > struct spmi_controller is a controller for a single bus. Inside your
> > device you have two SPMI buses, each can be controlled by its own
> > struct spmi_controller. Just like devices that control multiple I2C,
> > SPI or USB busses register a separate instance of the bus controller.
>
> Well, this is what this patchset is trying to do in the generic part.
> The SPMI controller supports multiple buses (HW-wise) and therefore SW
> implementation shouldn't be tied to single bus requirement.

So, after the off-line discussion:
- add new compatible string for sm8450+
- register two spmi controller instances
- drop the master-id from the SPMI interface
- optionally: think about having a new separate driver for v7 SPMI.

>
> >
> > >
> > > I'm not saying it might not work. But, to me, it looks more like a hack.
> > >
> > > Basically, we would be mapping HW bus masters to kernel controllers.
> >
> > Buses, not just masters.
> >
> > >
> > > >
> > > > >
> > > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > > > > ---
> > > > >  drivers/spmi/spmi-mtk-pmif.c |  6 ++--
> > > > >  drivers/spmi/spmi-pmic-arb.c | 10 +++---
> > > > >  drivers/spmi/spmi.c          | 76 ++++++++++++++++++++++++++++++--------------
> > > > >  include/linux/spmi.h         | 10 +++---
> > > > >  4 files changed, 67 insertions(+), 35 deletions(-)
> > > >
> > > > --
> > > > With best wishes
> > > > Dmitry
> >
> >
> >
> > --
> > With best wishes
> > Dmitry
AngeloGioacchino Del Regno Feb. 7, 2024, 12:46 p.m. UTC | #7 
Il 07/02/24 12:45, Dmitry Baryshkov ha scritto:
> On Wed, 7 Feb 2024 at 11:08, Abel Vesa <abel.vesa@linaro.org> wrote:
>>
>> On 24-02-07 09:23:09, Dmitry Baryshkov wrote:
>>> On Wed, 7 Feb 2024 at 09:19, Abel Vesa <abel.vesa@linaro.org> wrote:
>>>>
>>>> On 24-02-07 01:55:39, Dmitry Baryshkov wrote:
>>>>> On Wed, 7 Feb 2024 at 01:34, Abel Vesa <abel.vesa@linaro.org> wrote:
>>>>>>
>>>>>> Some newer SPMI controllers support multiple bus masters.
>>>>>> Such a master can control multiple slave devices. The generic
>>>>>> framework needs to be able to pass on the master id to the
>>>>>> controller-specific driver. So do that. The framework will
>>>>>> check if the devicetree child nodes are actually bus masters
>>>>>> and will register the devices for each master. The legacy
>>>>>> approach will still be supported for backwards compatibility.
>>>>>
>>>>> Please remind me, are those two actual bus musters driving a single
>>>>> bus in parallel or two SPMI buses being handled by a single device? In
>>>>> the latter case this implementation is incorrect. There should be
>>>>> multiple spmi_controller instances, one for each bus. Allocate them in
>>>>> a loop and set ctrl->dev.of_node after allocating.
>>>>
>>>> It's two SPMI buses (two sets of wires) handled by the same controller,
>>>> HW-wise.
>>>>
>>>> If we register two spmi controllers with the kernel framework, it will
>>>> be HW inaccurate, because there is just one controller which has
>>>> multiple masters.
>>>
>>> struct spmi_controller is a controller for a single bus. Inside your
>>> device you have two SPMI buses, each can be controlled by its own
>>> struct spmi_controller. Just like devices that control multiple I2C,
>>> SPI or USB busses register a separate instance of the bus controller.
>>
>> Well, this is what this patchset is trying to do in the generic part.
>> The SPMI controller supports multiple buses (HW-wise) and therefore SW
>> implementation shouldn't be tied to single bus requirement.
> 
> So, after the off-line discussion:
> - add new compatible string for sm8450+
> - register two spmi controller instances

Well, I don't know about the actual hardware that you're trying to implement
but, in my opinion, the "idea" of this series does actually make sense.

The SPMI specification says that SPMI supports up to 4 masters, and up to
16 slaves.

Just my two cents.

Cheers,
Angelo

> - drop the master-id from the SPMI interface
> - optionally: think about having a new separate driver for v7 SPMI.
> 
>>
>>>
>>>>
>>>> I'm not saying it might not work. But, to me, it looks more like a hack.
>>>>
>>>> Basically, we would be mapping HW bus masters to kernel controllers.
>>>
>>> Buses, not just masters.
>>>
>>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
>>>>>> ---
>>>>>>   drivers/spmi/spmi-mtk-pmif.c |  6 ++--
>>>>>>   drivers/spmi/spmi-pmic-arb.c | 10 +++---
>>>>>>   drivers/spmi/spmi.c          | 76 ++++++++++++++++++++++++++++++--------------
>>>>>>   include/linux/spmi.h         | 10 +++---
>>>>>>   4 files changed, 67 insertions(+), 35 deletions(-)
>>>>>
>>>>> --
>>>>> With best wishes
>>>>> Dmitry
>>>
>>>
>>>
>>> --
>>> With best wishes
>>> Dmitry
> 
> 
>
Dmitry Baryshkov Feb. 7, 2024, 2:22 p.m. UTC | #8 
On Wed, 7 Feb 2024 at 14:46, AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 07/02/24 12:45, Dmitry Baryshkov ha scritto:
> > On Wed, 7 Feb 2024 at 11:08, Abel Vesa <abel.vesa@linaro.org> wrote:
> >>
> >> On 24-02-07 09:23:09, Dmitry Baryshkov wrote:
> >>> On Wed, 7 Feb 2024 at 09:19, Abel Vesa <abel.vesa@linaro.org> wrote:
> >>>>
> >>>> On 24-02-07 01:55:39, Dmitry Baryshkov wrote:
> >>>>> On Wed, 7 Feb 2024 at 01:34, Abel Vesa <abel.vesa@linaro.org> wrote:
> >>>>>>
> >>>>>> Some newer SPMI controllers support multiple bus masters.
> >>>>>> Such a master can control multiple slave devices. The generic
> >>>>>> framework needs to be able to pass on the master id to the
> >>>>>> controller-specific driver. So do that. The framework will
> >>>>>> check if the devicetree child nodes are actually bus masters
> >>>>>> and will register the devices for each master. The legacy
> >>>>>> approach will still be supported for backwards compatibility.
> >>>>>
> >>>>> Please remind me, are those two actual bus musters driving a single
> >>>>> bus in parallel or two SPMI buses being handled by a single device? In
> >>>>> the latter case this implementation is incorrect. There should be
> >>>>> multiple spmi_controller instances, one for each bus. Allocate them in
> >>>>> a loop and set ctrl->dev.of_node after allocating.
> >>>>
> >>>> It's two SPMI buses (two sets of wires) handled by the same controller,
> >>>> HW-wise.
> >>>>
> >>>> If we register two spmi controllers with the kernel framework, it will
> >>>> be HW inaccurate, because there is just one controller which has
> >>>> multiple masters.
> >>>
> >>> struct spmi_controller is a controller for a single bus. Inside your
> >>> device you have two SPMI buses, each can be controlled by its own
> >>> struct spmi_controller. Just like devices that control multiple I2C,
> >>> SPI or USB busses register a separate instance of the bus controller.
> >>
> >> Well, this is what this patchset is trying to do in the generic part.
> >> The SPMI controller supports multiple buses (HW-wise) and therefore SW
> >> implementation shouldn't be tied to single bus requirement.
> >
> > So, after the off-line discussion:
> > - add new compatible string for sm8450+
> > - register two spmi controller instances
>
> Well, I don't know about the actual hardware that you're trying to implement
> but, in my opinion, the "idea" of this series does actually make sense.
>
> The SPMI specification says that SPMI supports up to 4 masters, and up to
> 16 slaves.

So, that is my main question: whether this supports multiple masters
on a same bus or multiple buses. From the SoC pins description I
assume the latter is the case.

>
> Just my two cents.
>
> Cheers,
> Angelo
>
> > - drop the master-id from the SPMI interface
> > - optionally: think about having a new separate driver for v7 SPMI.
> >
> >>
> >>>
> >>>>
> >>>> I'm not saying it might not work. But, to me, it looks more like a hack.
> >>>>
> >>>> Basically, we would be mapping HW bus masters to kernel controllers.
> >>>
> >>> Buses, not just masters.
> >>>
> >>>>
> >>>>>
> >>>>>>
> >>>>>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> >>>>>> ---
> >>>>>>   drivers/spmi/spmi-mtk-pmif.c |  6 ++--
> >>>>>>   drivers/spmi/spmi-pmic-arb.c | 10 +++---
> >>>>>>   drivers/spmi/spmi.c          | 76 ++++++++++++++++++++++++++++++--------------
> >>>>>>   include/linux/spmi.h         | 10 +++---
> >>>>>>   4 files changed, 67 insertions(+), 35 deletions(-)
> >>>>>
> >>>>> --
> >>>>> With best wishes
> >>>>> Dmitry
> >>>
> >>>
> >>>
> >>> --
> >>> With best wishes
> >>> Dmitry
> >
> >
> >
>
>
Neil Armstrong Feb. 7, 2024, 2:57 p.m. UTC | #9 
On 07/02/2024 15:22, Dmitry Baryshkov wrote:
> On Wed, 7 Feb 2024 at 14:46, AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Il 07/02/24 12:45, Dmitry Baryshkov ha scritto:
>>> On Wed, 7 Feb 2024 at 11:08, Abel Vesa <abel.vesa@linaro.org> wrote:
>>>>
>>>> On 24-02-07 09:23:09, Dmitry Baryshkov wrote:
>>>>> On Wed, 7 Feb 2024 at 09:19, Abel Vesa <abel.vesa@linaro.org> wrote:
>>>>>>
>>>>>> On 24-02-07 01:55:39, Dmitry Baryshkov wrote:
>>>>>>> On Wed, 7 Feb 2024 at 01:34, Abel Vesa <abel.vesa@linaro.org> wrote:
>>>>>>>>
>>>>>>>> Some newer SPMI controllers support multiple bus masters.
>>>>>>>> Such a master can control multiple slave devices. The generic
>>>>>>>> framework needs to be able to pass on the master id to the
>>>>>>>> controller-specific driver. So do that. The framework will
>>>>>>>> check if the devicetree child nodes are actually bus masters
>>>>>>>> and will register the devices for each master. The legacy
>>>>>>>> approach will still be supported for backwards compatibility.
>>>>>>>
>>>>>>> Please remind me, are those two actual bus musters driving a single
>>>>>>> bus in parallel or two SPMI buses being handled by a single device? In
>>>>>>> the latter case this implementation is incorrect. There should be
>>>>>>> multiple spmi_controller instances, one for each bus. Allocate them in
>>>>>>> a loop and set ctrl->dev.of_node after allocating.
>>>>>>
>>>>>> It's two SPMI buses (two sets of wires) handled by the same controller,
>>>>>> HW-wise.
>>>>>>
>>>>>> If we register two spmi controllers with the kernel framework, it will
>>>>>> be HW inaccurate, because there is just one controller which has
>>>>>> multiple masters.
>>>>>
>>>>> struct spmi_controller is a controller for a single bus. Inside your
>>>>> device you have two SPMI buses, each can be controlled by its own
>>>>> struct spmi_controller. Just like devices that control multiple I2C,
>>>>> SPI or USB busses register a separate instance of the bus controller.
>>>>
>>>> Well, this is what this patchset is trying to do in the generic part.
>>>> The SPMI controller supports multiple buses (HW-wise) and therefore SW
>>>> implementation shouldn't be tied to single bus requirement.
>>>
>>> So, after the off-line discussion:
>>> - add new compatible string for sm8450+
>>> - register two spmi controller instances
>>
>> Well, I don't know about the actual hardware that you're trying to implement
>> but, in my opinion, the "idea" of this series does actually make sense.
>>
>> The SPMI specification says that SPMI supports up to 4 masters, and up to
>> 16 slaves.
> 
> So, that is my main question: whether this supports multiple masters
> on a same bus or multiple buses. From the SoC pins description I
> assume the latter is the case.

This is clearly 2 separate physically separate busses, not 2 masters on the same bus.

We registers separate controllers for i2c, spi, 1w, ... in this case, why not here ?

Neil

> 
>>
>> Just my two cents.
>>
>> Cheers,
>> Angelo
>>
>>> - drop the master-id from the SPMI interface
>>> - optionally: think about having a new separate driver for v7 SPMI.
>>>
>>>>
>>>>>
>>>>>>
>>>>>> I'm not saying it might not work. But, to me, it looks more like a hack.
>>>>>>
>>>>>> Basically, we would be mapping HW bus masters to kernel controllers.
>>>>>
>>>>> Buses, not just masters.
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
>>>>>>>> ---
>>>>>>>>    drivers/spmi/spmi-mtk-pmif.c |  6 ++--
>>>>>>>>    drivers/spmi/spmi-pmic-arb.c | 10 +++---
>>>>>>>>    drivers/spmi/spmi.c          | 76 ++++++++++++++++++++++++++++++--------------
>>>>>>>>    include/linux/spmi.h         | 10 +++---
>>>>>>>>    4 files changed, 67 insertions(+), 35 deletions(-)
>>>>>>>
>>>>>>> --
>>>>>>> With best wishes
>>>>>>> Dmitry
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> With best wishes
>>>>> Dmitry
>>>
>>>
>>>
>>
>>
> 
>
diff mbox series

Patch

diff --git a/drivers/spmi/spmi-mtk-pmif.c b/drivers/spmi/spmi-mtk-pmif.c
index 5079442f8ea1..b19bb0351ff1 100644
--- a/drivers/spmi/spmi-mtk-pmif.c
+++ b/drivers/spmi/spmi-mtk-pmif.c
@@ -286,7 +286,7 @@  static bool pmif_is_fsm_vldclr(struct pmif *arb)
 	return GET_SWINF(reg_rdata) == SWINF_WFVLDCLR;
 }
 
-static int pmif_arb_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid)
+static int pmif_arb_cmd(struct spmi_controller *ctrl, u8 opc, u8 master_id, u8 sid)
 {
 	struct pmif *arb = spmi_controller_get_drvdata(ctrl);
 	u32 rdata, cmd;
@@ -308,7 +308,7 @@  static int pmif_arb_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid)
 	return ret;
 }
 
-static int pmif_spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
+static int pmif_spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 master_id, u8 sid,
 			      u16 addr, u8 *buf, size_t len)
 {
 	struct pmif *arb = spmi_controller_get_drvdata(ctrl);
@@ -375,7 +375,7 @@  static int pmif_spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 	return 0;
 }
 
-static int pmif_spmi_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
+static int pmif_spmi_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 master_id, u8 sid,
 			       u16 addr, const u8 *buf, size_t len)
 {
 	struct pmif *arb = spmi_controller_get_drvdata(ctrl);
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 9ed1180fe31f..597207720146 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -341,7 +341,7 @@  pmic_arb_non_data_cmd_v2(struct spmi_controller *ctrl, u8 opc, u8 sid)
 }
 
 /* Non-data command */
-static int pmic_arb_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid)
+static int pmic_arb_cmd(struct spmi_controller *ctrl, u8 opc, u8 master_id, u8 sid)
 {
 	struct spmi_pmic_arb *pmic_arb = spmi_controller_get_drvdata(ctrl);
 
@@ -410,7 +410,7 @@  static int pmic_arb_read_cmd_unlocked(struct spmi_controller *ctrl, u32 cmd,
 	return 0;
 }
 
-static int pmic_arb_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
+static int pmic_arb_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 master_id, u8 sid,
 			     u16 addr, u8 *buf, size_t len)
 {
 	struct spmi_pmic_arb *pmic_arb = spmi_controller_get_drvdata(ctrl);
@@ -486,7 +486,7 @@  static int pmic_arb_write_cmd_unlocked(struct spmi_controller *ctrl, u32 cmd,
 				      PMIC_ARB_CHANNEL_RW);
 }
 
-static int pmic_arb_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
+static int pmic_arb_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 master_id, u8 sid,
 			      u16 addr, const u8 *buf, size_t len)
 {
 	struct spmi_pmic_arb *pmic_arb = spmi_controller_get_drvdata(ctrl);
@@ -568,7 +568,7 @@  static void qpnpint_spmi_write(struct irq_data *d, u8 reg, void *buf,
 	u8 sid = hwirq_to_sid(d->hwirq);
 	u8 per = hwirq_to_per(d->hwirq);
 
-	if (pmic_arb_write_cmd(pmic_arb->spmic, SPMI_CMD_EXT_WRITEL, sid,
+	if (pmic_arb_write_cmd(pmic_arb->spmic, SPMI_CMD_EXT_WRITEL, 0, sid,
 			       (per << 8) + reg, buf, len))
 		dev_err_ratelimited(&pmic_arb->spmic->dev, "failed irqchip transaction on %x\n",
 				    d->irq);
@@ -580,7 +580,7 @@  static void qpnpint_spmi_read(struct irq_data *d, u8 reg, void *buf, size_t len)
 	u8 sid = hwirq_to_sid(d->hwirq);
 	u8 per = hwirq_to_per(d->hwirq);
 
-	if (pmic_arb_read_cmd(pmic_arb->spmic, SPMI_CMD_EXT_READL, sid,
+	if (pmic_arb_read_cmd(pmic_arb->spmic, SPMI_CMD_EXT_READL, 0, sid,
 			      (per << 8) + reg, buf, len))
 		dev_err_ratelimited(&pmic_arb->spmic->dev, "failed irqchip transaction on %x\n",
 				    d->irq);
diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c
index 3a60fd2e09e1..7dc778db7242 100644
--- a/drivers/spmi/spmi.c
+++ b/drivers/spmi/spmi.c
@@ -64,7 +64,7 @@  int spmi_device_add(struct spmi_device *sdev)
 	struct spmi_controller *ctrl = sdev->ctrl;
 	int err;
 
-	dev_set_name(&sdev->dev, "%d-%02x", ctrl->nr, sdev->usid);
+	dev_set_name(&sdev->dev, "%d-%02x-%02x", ctrl->nr, sdev->mid, sdev->usid);
 
 	err = device_add(&sdev->dev);
 	if (err < 0) {
@@ -91,19 +91,19 @@  void spmi_device_remove(struct spmi_device *sdev)
 EXPORT_SYMBOL_GPL(spmi_device_remove);
 
 static inline int
-spmi_cmd(struct spmi_controller *ctrl, u8 opcode, u8 sid)
+spmi_cmd(struct spmi_controller *ctrl, u8 opcode, u8 mid, u8 sid)
 {
 	int ret;
 
 	if (!ctrl || !ctrl->cmd || ctrl->dev.type != &spmi_ctrl_type)
 		return -EINVAL;
 
-	ret = ctrl->cmd(ctrl, opcode, sid);
+	ret = ctrl->cmd(ctrl, opcode, mid, sid);
 	trace_spmi_cmd(opcode, sid, ret);
 	return ret;
 }
 
-static inline int spmi_read_cmd(struct spmi_controller *ctrl, u8 opcode,
+static inline int spmi_read_cmd(struct spmi_controller *ctrl, u8 opcode, u8 mid,
 				u8 sid, u16 addr, u8 *buf, size_t len)
 {
 	int ret;
@@ -112,12 +112,12 @@  static inline int spmi_read_cmd(struct spmi_controller *ctrl, u8 opcode,
 		return -EINVAL;
 
 	trace_spmi_read_begin(opcode, sid, addr);
-	ret = ctrl->read_cmd(ctrl, opcode, sid, addr, buf, len);
+	ret = ctrl->read_cmd(ctrl, opcode, mid, sid, addr, buf, len);
 	trace_spmi_read_end(opcode, sid, addr, ret, len, buf);
 	return ret;
 }
 
-static inline int spmi_write_cmd(struct spmi_controller *ctrl, u8 opcode,
+static inline int spmi_write_cmd(struct spmi_controller *ctrl, u8 opcode, u8 mid,
 				 u8 sid, u16 addr, const u8 *buf, size_t len)
 {
 	int ret;
@@ -126,7 +126,7 @@  static inline int spmi_write_cmd(struct spmi_controller *ctrl, u8 opcode,
 		return -EINVAL;
 
 	trace_spmi_write_begin(opcode, sid, addr, len, buf);
-	ret = ctrl->write_cmd(ctrl, opcode, sid, addr, buf, len);
+	ret = ctrl->write_cmd(ctrl, opcode, mid, sid, addr, buf, len);
 	trace_spmi_write_end(opcode, sid, addr, ret);
 	return ret;
 }
@@ -145,7 +145,7 @@  int spmi_register_read(struct spmi_device *sdev, u8 addr, u8 *buf)
 	if (addr > 0x1F)
 		return -EINVAL;
 
-	return spmi_read_cmd(sdev->ctrl, SPMI_CMD_READ, sdev->usid, addr,
+	return spmi_read_cmd(sdev->ctrl, SPMI_CMD_READ, sdev->mid, sdev->usid, addr,
 			     buf, 1);
 }
 EXPORT_SYMBOL_GPL(spmi_register_read);
@@ -167,7 +167,7 @@  int spmi_ext_register_read(struct spmi_device *sdev, u8 addr, u8 *buf,
 	if (len == 0 || len > 16)
 		return -EINVAL;
 
-	return spmi_read_cmd(sdev->ctrl, SPMI_CMD_EXT_READ, sdev->usid, addr,
+	return spmi_read_cmd(sdev->ctrl, SPMI_CMD_EXT_READ, sdev->mid, sdev->usid, addr,
 			     buf, len);
 }
 EXPORT_SYMBOL_GPL(spmi_ext_register_read);
@@ -189,7 +189,7 @@  int spmi_ext_register_readl(struct spmi_device *sdev, u16 addr, u8 *buf,
 	if (len == 0 || len > 8)
 		return -EINVAL;
 
-	return spmi_read_cmd(sdev->ctrl, SPMI_CMD_EXT_READL, sdev->usid, addr,
+	return spmi_read_cmd(sdev->ctrl, SPMI_CMD_EXT_READL, sdev->mid, sdev->usid, addr,
 			     buf, len);
 }
 EXPORT_SYMBOL_GPL(spmi_ext_register_readl);
@@ -208,7 +208,7 @@  int spmi_register_write(struct spmi_device *sdev, u8 addr, u8 data)
 	if (addr > 0x1F)
 		return -EINVAL;
 
-	return spmi_write_cmd(sdev->ctrl, SPMI_CMD_WRITE, sdev->usid, addr,
+	return spmi_write_cmd(sdev->ctrl, SPMI_CMD_WRITE, sdev->mid, sdev->usid, addr,
 			      &data, 1);
 }
 EXPORT_SYMBOL_GPL(spmi_register_write);
@@ -222,7 +222,7 @@  EXPORT_SYMBOL_GPL(spmi_register_write);
  */
 int spmi_register_zero_write(struct spmi_device *sdev, u8 data)
 {
-	return spmi_write_cmd(sdev->ctrl, SPMI_CMD_ZERO_WRITE, sdev->usid, 0,
+	return spmi_write_cmd(sdev->ctrl, SPMI_CMD_ZERO_WRITE, sdev->mid, sdev->usid, 0,
 			      &data, 1);
 }
 EXPORT_SYMBOL_GPL(spmi_register_zero_write);
@@ -244,7 +244,7 @@  int spmi_ext_register_write(struct spmi_device *sdev, u8 addr, const u8 *buf,
 	if (len == 0 || len > 16)
 		return -EINVAL;
 
-	return spmi_write_cmd(sdev->ctrl, SPMI_CMD_EXT_WRITE, sdev->usid, addr,
+	return spmi_write_cmd(sdev->ctrl, SPMI_CMD_EXT_WRITE, sdev->mid, sdev->usid, addr,
 			      buf, len);
 }
 EXPORT_SYMBOL_GPL(spmi_ext_register_write);
@@ -266,7 +266,7 @@  int spmi_ext_register_writel(struct spmi_device *sdev, u16 addr, const u8 *buf,
 	if (len == 0 || len > 8)
 		return -EINVAL;
 
-	return spmi_write_cmd(sdev->ctrl, SPMI_CMD_EXT_WRITEL, sdev->usid,
+	return spmi_write_cmd(sdev->ctrl, SPMI_CMD_EXT_WRITEL, sdev->mid, sdev->usid,
 			      addr, buf, len);
 }
 EXPORT_SYMBOL_GPL(spmi_ext_register_writel);
@@ -281,7 +281,7 @@  EXPORT_SYMBOL_GPL(spmi_ext_register_writel);
  */
 int spmi_command_reset(struct spmi_device *sdev)
 {
-	return spmi_cmd(sdev->ctrl, SPMI_CMD_RESET, sdev->usid);
+	return spmi_cmd(sdev->ctrl, SPMI_CMD_RESET, sdev->mid, sdev->usid);
 }
 EXPORT_SYMBOL_GPL(spmi_command_reset);
 
@@ -293,7 +293,7 @@  EXPORT_SYMBOL_GPL(spmi_command_reset);
  */
 int spmi_command_sleep(struct spmi_device *sdev)
 {
-	return spmi_cmd(sdev->ctrl, SPMI_CMD_SLEEP, sdev->usid);
+	return spmi_cmd(sdev->ctrl, SPMI_CMD_SLEEP, sdev->mid, sdev->usid);
 }
 EXPORT_SYMBOL_GPL(spmi_command_sleep);
 
@@ -306,7 +306,7 @@  EXPORT_SYMBOL_GPL(spmi_command_sleep);
  */
 int spmi_command_wakeup(struct spmi_device *sdev)
 {
-	return spmi_cmd(sdev->ctrl, SPMI_CMD_WAKEUP, sdev->usid);
+	return spmi_cmd(sdev->ctrl, SPMI_CMD_WAKEUP, sdev->mid, sdev->usid);
 }
 EXPORT_SYMBOL_GPL(spmi_command_wakeup);
 
@@ -318,7 +318,7 @@  EXPORT_SYMBOL_GPL(spmi_command_wakeup);
  */
 int spmi_command_shutdown(struct spmi_device *sdev)
 {
-	return spmi_cmd(sdev->ctrl, SPMI_CMD_SHUTDOWN, sdev->usid);
+	return spmi_cmd(sdev->ctrl, SPMI_CMD_SHUTDOWN, sdev->mid, sdev->usid);
 }
 EXPORT_SYMBOL_GPL(spmi_command_shutdown);
 
@@ -477,15 +477,16 @@  struct spmi_controller *spmi_controller_alloc(struct device *parent,
 }
 EXPORT_SYMBOL_GPL(spmi_controller_alloc);
 
-static void of_spmi_register_devices(struct spmi_controller *ctrl)
+static void of_spmi_register_devices(struct spmi_controller *ctrl,
+				     struct device_node *parent, u8 mid)
 {
 	struct device_node *node;
 	int err;
 
-	if (!ctrl->dev.of_node)
+	if (!parent)
 		return;
 
-	for_each_available_child_of_node(ctrl->dev.of_node, node) {
+	for_each_available_child_of_node(parent, node) {
 		struct spmi_device *sdev;
 		u32 reg[2];
 
@@ -519,6 +520,7 @@  static void of_spmi_register_devices(struct spmi_controller *ctrl)
 
 		sdev->dev.of_node = node;
 		sdev->usid = (u8)reg[0];
+		sdev->mid = mid;
 
 		err = spmi_device_add(sdev);
 		if (err) {
@@ -529,6 +531,30 @@  static void of_spmi_register_devices(struct spmi_controller *ctrl)
 	}
 }
 
+static int of_spmi_register_bus_masters(struct spmi_controller *ctrl)
+{
+	struct device_node *node = ctrl->dev.of_node, *child;
+	int mid = 0;
+
+	for_each_available_child_of_node(node, child) {
+		if (of_node_name_eq(child, "spmi-bus-master"))
+			of_spmi_register_devices(ctrl, child, mid++);
+	}
+
+	return 0;
+}
+
+static bool of_spmi_has_bus_multi_master(struct spmi_controller *ctrl)
+{
+	struct device_node *node = ctrl->dev.of_node, *child;
+
+	for_each_available_child_of_node(node, child)
+		if (of_node_name_eq(child, "spmi-bus-master"))
+			return true;
+
+	return false;
+}
+
 /**
  * spmi_controller_add() - Add an SPMI controller
  * @ctrl:	controller to be registered.
@@ -548,8 +574,12 @@  int spmi_controller_add(struct spmi_controller *ctrl)
 	if (ret)
 		return ret;
 
-	if (IS_ENABLED(CONFIG_OF))
-		of_spmi_register_devices(ctrl);
+	if (IS_ENABLED(CONFIG_OF)) {
+		if (of_spmi_has_bus_multi_master(ctrl))
+			of_spmi_register_bus_masters(ctrl);
+		else
+			of_spmi_register_devices(ctrl, ctrl->dev.of_node, 0);
+	}
 
 	dev_dbg(&ctrl->dev, "spmi-%d registered: dev:%p\n",
 		ctrl->nr, &ctrl->dev);
diff --git a/include/linux/spmi.h b/include/linux/spmi.h
index 28e8c8bd3944..6e9031df47f0 100644
--- a/include/linux/spmi.h
+++ b/include/linux/spmi.h
@@ -34,12 +34,14 @@ 
  * struct spmi_device - Basic representation of an SPMI device
  * @dev:	Driver model representation of the device.
  * @ctrl:	SPMI controller managing the bus hosting this device.
- * @usid:	This devices' Unique Slave IDentifier.
+ * @usid:	This device's Unique Slave IDentifier.
+ * @mid:	This device's Bus Master IDentifier.
  */
 struct spmi_device {
 	struct device		dev;
 	struct spmi_controller	*ctrl;
 	u8			usid;
+	u8			mid;
 };
 
 static inline struct spmi_device *to_spmi_device(struct device *d)
@@ -80,10 +82,10 @@  void spmi_device_remove(struct spmi_device *sdev);
 struct spmi_controller {
 	struct device		dev;
 	unsigned int		nr;
-	int	(*cmd)(struct spmi_controller *ctrl, u8 opcode, u8 sid);
-	int	(*read_cmd)(struct spmi_controller *ctrl, u8 opcode,
+	int	(*cmd)(struct spmi_controller *ctrl, u8 opcode, u8 mid, u8 sid);
+	int	(*read_cmd)(struct spmi_controller *ctrl, u8 opcode, u8 mid,
 			    u8 sid, u16 addr, u8 *buf, size_t len);
-	int	(*write_cmd)(struct spmi_controller *ctrl, u8 opcode,
+	int	(*write_cmd)(struct spmi_controller *ctrl, u8 opcode, u8 mid,
 			     u8 sid, u16 addr, const u8 *buf, size_t len);
 };