Re: [PATCH] dt-bindings: fsl: fsl,rcpm: fix unevaluated fsl,rcpm-wakeup property

[Conor Dooley]

On Sun, Aug 11, 2024 at 05:35:07PM +0200, Krzysztof Kozlowski wrote:
> Drop the RCPM consumer example from the binding (LPUART device node),
> because:
> 1. Using phandles is typical syntax, thus explaining it is not needed in
>the provider binding,
> 2. It has 'fsl,rcpm-wakeup' property which is not allowed by LPUART
>binding so it causes dt_binding_check warning:
> 
>fsl,rcpm.example.dtb: serial@295: Unevaluated properties are not 
> allowed ('fsl,rcpm-wakeup' was unexpected)
>  from schema $id: http://devicetree.org/schemas/serial/fsl-lpuart.yaml#
> 
> Alternatively, this property could be added to LPUART binding
> (fsl-lpuart.yaml), but it looks like none of in-tree DTS use it.
> 
> Fixes: ad21e3840a88 ("dt-bindings: soc: fsl: Convert rcpm to yaml format")
> Signed-off-by: Krzysztof Kozlowski 

Acked-by: Conor Dooley 


signature.asc
Description: PGP signature

Re: [External] [PATCH RFC/RFT v2 3/4] riscv: Stop emitting preventive sfence.vma for new vmalloc mappings

[Conor Dooley]

On Tue, Jun 04, 2024 at 01:44:15PM +0200, Alexandre Ghiti wrote:
> On Tue, Jun 4, 2024 at 10:52 AM Conor Dooley  wrote:
> >
> > On Tue, Jun 04, 2024 at 09:17:26AM +0200, Alexandre Ghiti wrote:
> > > On Tue, Jun 4, 2024 at 9:15 AM Alexandre Ghiti  
> > > wrote:
> > > > On Tue, Jun 4, 2024 at 8:21 AM yunhui cui  
> > > > wrote:
> > > > >
> > > > > As for the current status of the patch, there are two points that can
> > > > > be optimized:
> > > > > 1. Some chip hardware implementations may not cache TLB invalid
> > > > > entries, so it doesn't matter whether svvptc is available or not. Can
> > > > > we consider adding a CONFIG_RISCV_SVVPTC to control it?
> > >
> > > That would produce a non-portable kernel. But I'm not opposed to that
> > > at all, let me check how we handle other extensions. Maybe @Conor
> > > Dooley has some feedback here?
> >
> > To be honest, not really sure what to give feedback on. Could you
> > elaborate on exactly what the option is going to do? Given the
> > portability concern, I guess you were proposing that the option would
> > remove the preventative fences, rather than your current patch that
> > removes them via an alternative?
> 
> No no, I won't do that, we need a generic kernel for distros so that's
> not even a question. What Yunhui was asking about (to me) is: can we
> introduce a Kconfig option to always remove the preventive fences,
> bypassing the use of alternatives altogether?
> 
> To me, it won't make a difference in terms of performance. But if we
> already offer such a possibility for other extensions, well I'll do
> it. Otherwise, the question is: should we start doing that?

We don't do that for other extensions yet, because currently all the
extensions we have options for are additive. There's like 3 alternative
patchsites, and they are all just one nop? I don't see the point of
having a Kconfig knob for that.


signature.asc
Description: PGP signature

Re: [External] [PATCH RFC/RFT v2 3/4] riscv: Stop emitting preventive sfence.vma for new vmalloc mappings

[Conor Dooley]

On Tue, Jun 04, 2024 at 09:17:26AM +0200, Alexandre Ghiti wrote:
> On Tue, Jun 4, 2024 at 9:15 AM Alexandre Ghiti  wrote:
> > On Tue, Jun 4, 2024 at 8:21 AM yunhui cui  wrote:
> > >
> > > As for the current status of the patch, there are two points that can
> > > be optimized:
> > > 1. Some chip hardware implementations may not cache TLB invalid
> > > entries, so it doesn't matter whether svvptc is available or not. Can
> > > we consider adding a CONFIG_RISCV_SVVPTC to control it?
> 
> That would produce a non-portable kernel. But I'm not opposed to that
> at all, let me check how we handle other extensions. Maybe @Conor
> Dooley has some feedback here?

To be honest, not really sure what to give feedback on. Could you
elaborate on exactly what the option is going to do? Given the
portability concern, I guess you were proposing that the option would
remove the preventative fences, rather than your current patch that
removes them via an alternative? I don't think we have any extension
related options that work like that at the moment, and making that an
option will just mean that distros that look to cater for multiple
platforms won't be able to turn it on.

Thanks,
Conor.


signature.asc
Description: PGP signature

Re: [PATCH 2/4] ASoC: dt-bindings: fsl,xcvr: Add two PLL clock sources

[Conor Dooley]

On Fri, May 10, 2024 at 10:38:30AM +0800, Shengjiu Wang wrote:
> On Fri, May 10, 2024 at 10:27 AM Shengjiu Wang  
> wrote:
> >
> > On Fri, May 10, 2024 at 1:14 AM Conor Dooley  wrote:
> > >
> > > On Thu, May 09, 2024 at 10:57:38AM +0800, Shengjiu Wang wrote:
> > > > Add two PLL clock sources, they are the parent clocks of the root clock
> > > > one is for 8kHz series rates, named as 'pll8k', another one is for
> > > > 11kHz series rates, named as 'pll11k'. They are optional clocks,
> > > > if there are such clocks, then the driver can switch between them to
> > > > support more accurate sample rates.
> > > >
> > > > As 'pll8k' and 'pll11k' are optional, then add 'minItems: 4' for
> > > > clocks and clock-names properties.
> > >
> > > Despite the detail given here in the commit message, the series this is
> > > appearing in and one of the driver patches makes me a bit "suspicious"
> > > of this patch. Are these newly added clocks available on all devices, or
> > > just on the imx95, or?
> >
> > These newly added clocks are only available for the imx95 XCVR.
> >
> 
> Looks like I should merge patch1 & 2 together, patch 3 & 3 together. right?

Please, and also add constraints so that the newly added clocks are only
allowed on the imx95.

Thanks,
Conor.


signature.asc
Description: PGP signature

Re: [PATCH 2/4] ASoC: dt-bindings: fsl,xcvr: Add two PLL clock sources

[Conor Dooley]

On Thu, May 09, 2024 at 10:57:38AM +0800, Shengjiu Wang wrote:
> Add two PLL clock sources, they are the parent clocks of the root clock
> one is for 8kHz series rates, named as 'pll8k', another one is for
> 11kHz series rates, named as 'pll11k'. They are optional clocks,
> if there are such clocks, then the driver can switch between them to
> support more accurate sample rates.
> 
> As 'pll8k' and 'pll11k' are optional, then add 'minItems: 4' for
> clocks and clock-names properties.

Despite the detail given here in the commit message, the series this is
appearing in and one of the driver patches makes me a bit "suspicious"
of this patch. Are these newly added clocks available on all devices, or
just on the imx95, or?

Thanks,
Conor.

> 
> Signed-off-by: Shengjiu Wang 
> ---
>  Documentation/devicetree/bindings/sound/fsl,xcvr.yaml | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/fsl,xcvr.yaml 
> b/Documentation/devicetree/bindings/sound/fsl,xcvr.yaml
> index 1c74a32def09..c4660faed404 100644
> --- a/Documentation/devicetree/bindings/sound/fsl,xcvr.yaml
> +++ b/Documentation/devicetree/bindings/sound/fsl,xcvr.yaml
> @@ -50,6 +50,9 @@ properties:
>- description: PHY clock
>- description: SPBA clock
>- description: PLL clock
> +  - description: PLL clock source for 8kHz series
> +  - description: PLL clock source for 11kHz series
> +minItems: 4
>  
>clock-names:
>  items:
> @@ -57,6 +60,9 @@ properties:
>- const: phy
>- const: spba
>- const: pll_ipg
> +  - const: pll8k
> +  - const: pll11k
> +minItems: 4
>  
>dmas:
>  items:
> -- 
> 2.34.1
> 


signature.asc
Description: PGP signature

Re: [PATCH 1/4] ASoC: dt-bindings: fsl,xcvr: Add compatible string for i.MX95

[Conor Dooley]

On Thu, May 09, 2024 at 10:57:37AM +0800, Shengjiu Wang wrote:
> Add compatible string "fsl,imx95-xcvr" for i.MX95 platform.

That's apparent from the diff. Why is it not compatible with existing
devices?

Cheers,
Conor.

> 
> Signed-off-by: Shengjiu Wang 
> ---
>  Documentation/devicetree/bindings/sound/fsl,xcvr.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/fsl,xcvr.yaml 
> b/Documentation/devicetree/bindings/sound/fsl,xcvr.yaml
> index 0eb0c1ba8710..1c74a32def09 100644
> --- a/Documentation/devicetree/bindings/sound/fsl,xcvr.yaml
> +++ b/Documentation/devicetree/bindings/sound/fsl,xcvr.yaml
> @@ -22,6 +22,7 @@ properties:
>  enum:
>- fsl,imx8mp-xcvr
>- fsl,imx93-xcvr
> +  - fsl,imx95-xcvr
>  
>reg:
>  items:
> -- 
> 2.34.1
> 


signature.asc
Description: PGP signature

Re: [PATCH v2 4/5] dt-bindings: hwmon: stts751: convert to dtschema

[Conor Dooley]

On Fri, Mar 22, 2024 at 07:45:29AM +0100, Javier Carrasco wrote:
> Convert existing binding to support validation.
> 
> This is a straightforward conversion with no new properties.
> 

Reviewed-by: Conor Dooley 

Thanks,
Conor.


signature.asc
Description: PGP signature

Re: [PATCH v2 2/5] dt-bindings: hwmon: ibmpowernv: convert to dtschema

[Conor Dooley]

On Fri, Mar 22, 2024 at 07:45:27AM +0100, Javier Carrasco wrote:
> Convert existing binding to support validation.
> 
> This is a straightforward conversion with now new properties.
> 
> Signed-off-by: Javier Carrasco 
> ---
>  .../devicetree/bindings/hwmon/ibm,powernv.yaml | 37 
> ++
>  .../devicetree/bindings/hwmon/ibmpowernv.txt   | 23 --
>  2 files changed, 37 insertions(+), 23 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ibm,powernv.yaml 
> b/Documentation/devicetree/bindings/hwmon/ibm,powernv.yaml
> new file mode 100644
> index ..b26d42bce938
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ibm,powernv.yaml

Filename should really match one of the compatibles. "powernv" is not a
type of hwmon as far as a quick google reports so I think its a poor
choice here regardless. With a compatible for the filename:
Reviewed-by: Conor Dooley 

Thanks,
Conor.

> @@ -0,0 +1,37 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/ibm,powernv.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: IBM POWERNV platform sensors
> +
> +maintainers:
> +  - Javier Carrasco 
> +
> +properties:
> +  compatible:
> +enum:
> +  - ibm,opal-sensor-cooling-fan
> +  - ibm,opal-sensor-amb-temp
> +  - ibm,opal-sensor-power-supply
> +  - ibm,opal-sensor-power
> +
> +  sensor-id:
> +description:
> +  An opaque id provided by the firmware to the kernel, identifies a
> +  given sensor and its attribute data.
> +$ref: /schemas/types.yaml#/definitions/uint32
> +
> +required:
> +  - compatible
> +  - sensor-id
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +sensor {
> +compatible = "ibm,opal-sensor-cooling-fan";
> +sensor-id = <0x7052107>;
> +};
> diff --git a/Documentation/devicetree/bindings/hwmon/ibmpowernv.txt 
> b/Documentation/devicetree/bindings/hwmon/ibmpowernv.txt
> deleted file mode 100644
> index f93242be60a1..
> --- a/Documentation/devicetree/bindings/hwmon/ibmpowernv.txt
> +++ /dev/null
> @@ -1,23 +0,0 @@
> -IBM POWERNV platform sensors
> -
> -
> -Required node properties:
> -- compatible: must be one of
> - "ibm,opal-sensor-cooling-fan"
> - "ibm,opal-sensor-amb-temp"
> - "ibm,opal-sensor-power-supply"
> - "ibm,opal-sensor-power"
> -- sensor-id: an opaque id provided by the firmware to the kernel, identifies 
> a
> -  given sensor and its attribute data
> -
> -Example sensors node:
> -
> -cooling-fan#8-data {
> - sensor-id = <0x7052107>;
> - compatible = "ibm,opal-sensor-cooling-fan";
> -};
> -
> -amb-temp#1-thrs {
> - sensor-id = <0x5096000>;
> - compatible = "ibm,opal-sensor-amb-temp";
> -};
> 
> -- 
> 2.40.1
> 


signature.asc
Description: PGP signature

Re: [PATCH 1/2] powerpc/prom_init: Replace linux,sml-base/sml-size with linux,sml-log

[Conor Dooley]

On Thu, Mar 07, 2024 at 04:15:01PM -0500, Stefan Berger wrote:
> 
> 
> On 3/7/24 15:39, Conor Dooley wrote:
> > On Thu, Mar 07, 2024 at 10:11:03AM -0500, Stefan Berger wrote:
> > > On 3/7/24 05:41, Michael Ellerman wrote:
> > > > Stefan Berger  writes:
> > 
> > > > 
> > > diff --git a/Documentation/devicetree/bindings/tpm/tpm-common.yaml
> > > b/Documentation/devicetree/bindings/tpm/tpm-common.yaml
> > > index 3c1241b2a43f..591c48f8cb74 100644
> > > --- a/Documentation/devicetree/bindings/tpm/tpm-common.yaml
> > > +++ b/Documentation/devicetree/bindings/tpm/tpm-common.yaml
> > > @@ -30,6 +30,11 @@ properties:
> > > size of reserved memory allocated for firmware event log
> > >   $ref: /schemas/types.yaml#/definitions/uint32
> > > 
> > > +  linux,sml-log:
> > > +description:
> > > +  firmware event log
> > 
> > Can you provide a more complete description here please as to what the
> > different between this and the other property? If I was populating a DT
> > I would have absolutely no idea whether or not to use this or the other
> > property, nor how to go about actually populating it.
> > The "log" in your example doesn't look like an actual log of any sort,
> > but I know nothing about TPMs so I'll take your word for it that that's
> > what a TPM log looks like.
> 
> In the example I cannot give a log but only a part of it. The log is in
> binary format and in case of TPM 2.0 starts with a header followed by log
> entries about what was measured. I don't think it's necessary to even give
> the full log header here. You do need some TPM specific knowledge about the
> 'firmware even log'.
> 
> 
> The existing properties are described like this:
> 
>   linux,sml-base:
> description:
>   base address of reserved memory allocated for firmware event log
> $ref: /schemas/types.yaml#/definitions/uint64
> 
>   linux,sml-size:
> description:
>   size of reserved memory allocated for firmware event log
> $ref: /schemas/types.yaml#/definitions/uint32
> 
> Would this describe the new property 'better' by prefixing it with
> 'embedded'?

IMO, no that's not any better. Spell it out so that someone who doesn't
know his arse from his elbow when it comes to tpm immediately knows that
this means the entire tpm log is inside the dtb. The paragraph you wrote
above gives more information about what this property is populated with
than the property description does.

>   linux,sml-log:
> description:
>   embedded firmware event log
> $ref: /schemas/types.yaml#/definitions/uint8-array
> 
> 
> > 
> > > +$ref: /schemas/types.yaml#/definitions/uint8-array
> > > +
> > > memory-region:
> > >   description: reserved memory allocated for firmware event log
> > >   maxItems: 1
> > > 
> > > 
> > > Is my patch missing something?
> > 
> > I think you also need the dependantSchema stuff you had in your original
> > snippet that makes the linux,* properties mutually exclusive with
> > memory-region (or at least something like that).
> > 
> I modified my new example now like this:
> ...
> ibm,loc-code = "U9080.HEX.134CA08-V7-C3";
> linux,sml-log = <00 00 00 00 03 00 00>;
> linux,sml-size = <0xbce10200>;   <-- added

> ibm,loc-code = "U8286.41A.10082DV-V3-C3";
> linux,sml-base = <0xc60e 0x0>;
> linux,sml-size = <0xbce10200>;
> linux,sml-log = <00 00 00 00 03 00 00>;   <- added
> 
> It errors out on bad examples, which is good.

Aye, that is covered by your new oneOf for this one binding. The
dependantSchema bit in tpm-common.yaml enforces it for all tpm devices.
It also covers the memory-region property being mutually exclusive with
the linux,sml-{base,size} properties so I think you need to extend that
to also cover linux,sml-lof property.

> > Please make sure you CC the DT maintainers and list on the v2 and Lukas
> > Wunner too.
> 
> Yes, I have them already cc'ed here.

To: Conor Dooley 
Cc: Michael Ellerman , linux-integr...@vger.kernel.org, 
linuxppc-dev@lists.ozlabs.org, conor.doo...@microchip.com, na...@linux.ibm.com, 
Lukas Wunner , linux-ker...@vger.kernel.org, jar...@kernel.org,
rnsas...@linux.ibm.com, peterhu...@gmx.de, vipar...@in.ibm.com

You have Lukas, one of the three DT maintainers and not the list as far
as I can see. Correct me please if I am wrong.

Thanks,
Conor.


signature.asc
Description: PGP signature

Re: [PATCH 1/2] powerpc/prom_init: Replace linux,sml-base/sml-size with linux,sml-log

[Conor Dooley]

On Thu, Mar 07, 2024 at 10:11:03AM -0500, Stefan Berger wrote:
> On 3/7/24 05:41, Michael Ellerman wrote:
> > Stefan Berger  writes:

> > 
> > Also adding the new linux,sml-log property should be accompanied by a
> > change to the device tree binding.
> 
> 
> See my proposal below.
> 
> > 
> > The syntax is not very obvious to me, but possibly something like?
> > 
> > diff --git a/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml 
> > b/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml
> > index 50a3fd31241c..cd75037948bc 100644
> > --- a/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml
> > +++ b/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml
> > @@ -74,8 +74,6 @@ required:
> > - ibm,my-dma-window
> > - ibm,my-drc-index
> > - ibm,loc-code
> > -  - linux,sml-base
> > -  - linux,sml-size
> >   allOf:
> > - $ref: tpm-common.yaml#
> > diff --git a/Documentation/devicetree/bindings/tpm/tpm-common.yaml 
> > b/Documentation/devicetree/bindings/tpm/tpm-common.yaml
> > index 3c1241b2a43f..616604707c95 100644
> > --- a/Documentation/devicetree/bindings/tpm/tpm-common.yaml
> > +++ b/Documentation/devicetree/bindings/tpm/tpm-common.yaml
> > @@ -25,6 +25,11 @@ properties:
> > base address of reserved memory allocated for firmware event log
> >   $ref: /schemas/types.yaml#/definitions/uint64
> > +  linux,sml-log:
> > +description:
> > +  Content of firmware event log
> > +$ref: /schemas/types.yaml#/definitions/uint8-array
> > +
> > linux,sml-size:
> >   description:
> > size of reserved memory allocated for firmware event log
> > @@ -53,15 +58,22 @@ dependentRequired:
> > linux,sml-base: ['linux,sml-size']
> > linux,sml-size: ['linux,sml-base']
> > -# must only have either memory-region or linux,sml-base
> > +# must only have either memory-region or linux,sml-base/size or 
> > linux,sml-log
> >   # as well as either resets or reset-gpios
> >   dependentSchemas:
> > memory-region:
> >   properties:
> > linux,sml-base: false
> > +  linux,sml-log: false
> > linux,sml-base:
> >   properties:
> > memory-region: false
> > +  linux,sml-log: false
> > +  linux,sml-log:
> > +properties:
> > +  memory-region: false
> > +  linux,sml-base: false
> > +  linux,sml-size: false
> > resets:
> >   properties:
> > reset-gpios: false
> > 
> > 
> 
> I have been working with this patch here now and it passes the following
> test:
> 
> make dt_binding_check dtbs_check DT_SCHEMA_FILES=tpm/ibm,vtpm.yaml
> 
> 
> diff --git a/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml
> b/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml
> index 50a3fd31241c..cacf6c3082de 100644
> --- a/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml
> +++ b/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml
> @@ -74,8 +74,12 @@ required:
>- ibm,my-dma-window
>- ibm,my-drc-index
>- ibm,loc-code
> -  - linux,sml-base
> -  - linux,sml-size
> +oneOf:
> +  - required:
> +  - linux,sml-base
> +  - linux,sml-size
> +  - required:
> +  - linux,sml-log
> 
>  allOf:
>- $ref: tpm-common.yaml#
> @@ -102,3 +106,21 @@ examples:
>  linux,sml-size = <0xbce10200>;
>  };
>  };
> +  - |
> +soc {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +
> +tpm@3003 {
> +compatible = "IBM,vtpm";
> +device_type = "IBM,vtpm";
> +reg = <0x3003>;
> +interrupts = <0xa0003 0x0>;
> +ibm,#dma-address-cells = <0x2>;
> +ibm,#dma-size-cells = <0x2>;
> +ibm,my-dma-window = <0x1003 0x0 0x0 0x0 0x1000>;
> +ibm,my-drc-index = <0x3003>;
> +ibm,loc-code = "U8286.41A.10082DV-V3-C3";
> +linux,sml-log = <00 00 00 00 03 00 00>;
> +};
> +};
> diff --git a/Documentation/devicetree/bindings/tpm/tpm-common.yaml
> b/Documentation/devicetree/bindings/tpm/tpm-common.yaml
> index 3c1241b2a43f..591c48f8cb74 100644
> --- a/Documentation/devicetree/bindings/tpm/tpm-common.yaml
> +++ b/Documentation/devicetree/bindings/tpm/tpm-common.yaml
> @@ -30,6 +30,11 @@ properties:
>size of reserved memory allocated for firmware event log
>  $ref: /schemas/types.yaml#/definitions/uint32
> 
> +  linux,sml-log:
> +description:
> +  firmware event log

Can you provide a more complete description here please as to what the
different between this and the other property? If I was populating a DT
I would have absolutely no idea whether or not to use this or the other
property, nor how to go about actually populating it.
The "log" in your example doesn't look like an actual log of any sort,
but I know nothing about TPMs so I'll take your word for it that that's
what a TPM log looks like.

> +$ref: /schemas/types.yaml#/definitions/uint8-array
> +
>memory-region:
>  description: reserved memory allocated for firmware event log
>  maxIt

Re: [PATCH 2/2] tpm: of: If available Use linux,sml-log to get the log and its size

[Conor Dooley]

On Wed, Mar 06, 2024 at 10:55:11AM -0500, Stefan Berger wrote:
> If linux,sml-log is available use it to get the TPM log rather than the
> pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM
> on Power where after a kexec the memory pointed to by linux,sml-base may
> have been corrupted. Also, linux,sml-log has replaced linux,sml-base and
> linux,sml-size on these two platforms.

Those two properties are documented, but linux,sml-log is not, nor can I
find patches on the list documenting it.
There should be a patch adding this to tmp-common.yaml.

Cheers,
Conor.

> Signed-off-by: Stefan Berger 
> ---
>  drivers/char/tpm/eventlog/of.c | 36 +++---
>  1 file changed, 11 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
> index 930fe43d5daf..e37196e64ef1 100644
> --- a/drivers/char/tpm/eventlog/of.c
> +++ b/drivers/char/tpm/eventlog/of.c
> @@ -54,8 +54,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
>   const u32 *sizep;
>   const u64 *basep;
>   struct tpm_bios_log *log;
> + const void *logp;
>   u32 size;
> - u64 base;
>  
>   log = &chip->log;
>   if (chip->dev.parent && chip->dev.parent->of_node)
> @@ -66,37 +66,23 @@ int tpm_read_log_of(struct tpm_chip *chip)
>   if (of_property_read_bool(np, "powered-while-suspended"))
>   chip->flags |= TPM_CHIP_FLAG_ALWAYS_POWERED;
>  
> - sizep = of_get_property(np, "linux,sml-size", NULL);
> - basep = of_get_property(np, "linux,sml-base", NULL);
> - if (sizep == NULL && basep == NULL)
> - return tpm_read_log_memory_region(chip);
> - if (sizep == NULL || basep == NULL)
> - return -EIO;
> -
> - /*
> -  * For both vtpm/tpm, firmware has log addr and log size in big
> -  * endian format. But in case of vtpm, there is a method called
> -  * sml-handover which is run during kernel init even before
> -  * device tree is setup. This sml-handover function takes care
> -  * of endianness and writes to sml-base and sml-size in little
> -  * endian format. For this reason, vtpm doesn't need conversion
> -  * but physical tpm needs the conversion.
> -  */
> - if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
> - of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
> + logp = of_get_property(np, "linux,sml-log", &size);
> + if (logp == NULL) {
> + sizep = of_get_property(np, "linux,sml-size", NULL);
> + basep = of_get_property(np, "linux,sml-base", NULL);
> + if (sizep == NULL && basep == NULL)
> + return tpm_read_log_memory_region(chip);
> + if (sizep == NULL || basep == NULL)
> + return -EIO;
> + logp = __va(be64_to_cpup((__force __be64 *)basep));
>   size = be32_to_cpup((__force __be32 *)sizep);
> - base = be64_to_cpup((__force __be64 *)basep);
> - } else {
> - size = *sizep;
> - base = *basep;
>   }
> -
>   if (size == 0) {
>   dev_warn(&chip->dev, "%s: Event log area empty\n", __func__);
>   return -EIO;
>   }
>  
> - log->bios_event_log = devm_kmemdup(&chip->dev, __va(base), size, 
> GFP_KERNEL);
> + log->bios_event_log = devm_kmemdup(&chip->dev, logp, size, GFP_KERNEL);
>   if (!log->bios_event_log)
>   return -ENOMEM;
>  
> -- 
> 2.43.0
> 


signature.asc
Description: PGP signature

Re: [PATCH 0/4] arm64: mm: support dynamic vmalloc/pmd configuration

[Conor Dooley]

Hey Maxwell,

FYI:

>   mm/vmalloc: allow arch-specific vmalloc_node overrides
>   mm: pgalloc: support address-conditional pmd allocation

With these two arch/riscv/configs/* are broken with calls to undeclared
functions.

>   arm64: separate code and data virtual memory allocation
>   arm64: dynamic enforcement of pmd-level PXNTable

And with these two the 32-bit and nommu builds are broken.

Cheers,
Conor.


signature.asc
Description: PGP signature

Re: [PATCH] tty: hvc: Don't enable the RISC-V SBI console by default

[Conor Dooley]

On Wed, Feb 14, 2024 at 07:34:30AM -0800, Palmer Dabbelt wrote:
> From: Palmer Dabbelt 
> 
> The new SBI console has the same problem as the old one: there's only
> one shared backing hardware and no synchronization, so the two drivers
> end up stepping on each other.  This was the same issue the old SBI-0.1
> console drivers had, but that was disabled by default when SBI-0.1 was.
> 
> So just mark the new driver as nonportable.
> 
> Reported-by: Emil Renner Berthing 
> Fixes: 88ead68e764c ("tty: Add SBI debug console support to HVC SBI driver")
> Signed-off-by: Palmer Dabbelt 

As was brought up when we covered this earlier today, if you're going to
probe a driver based on an ecall, the same hardware should not remain
enabled in the DT passed to the kernel.
If you want to enable this driver in a multiplatform kernel alongside
"real" drivers, then the solution is simple, firmware needs implementation
needs to patch the DT and, at least, mark the uart as reserved if it is
using it to provide the debug console. Marking this nonportable so that
people only walk into this with their eyes open seems like a reasonable
action to me.

Reviewed-by: Conor Dooley 

Cheers,
Conor.

> ---
>  drivers/tty/hvc/Kconfig | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/hvc/Kconfig b/drivers/tty/hvc/Kconfig
> index 6e05c5c7bca1..c2a4e88b328f 100644
> --- a/drivers/tty/hvc/Kconfig
> +++ b/drivers/tty/hvc/Kconfig
> @@ -108,13 +108,15 @@ config HVC_DCC_SERIALIZE_SMP
>  
>  config HVC_RISCV_SBI
>   bool "RISC-V SBI console support"
> - depends on RISCV_SBI
> + depends on RISCV_SBI && NONPORTABLE
>   select HVC_DRIVER
>   help
> This enables support for console output via RISC-V SBI calls, which
> -   is normally used only during boot to output printk.
> +   is normally used only during boot to output printk.  This driver
> +   conflicts with real console drivers and should not be enabled on
> +   systems that directly access the console.
>  
> -   If you don't know what do to here, say Y.
> +   If you don't know what do to here, say N.
>  
>  config HVCS
>   tristate "IBM Hypervisor Virtual Console Server support"
> -- 
> 2.43.0
> 
> 


signature.asc
Description: PGP signature

Re: [PATCH 2/3] arch and include: Update LLVM Phabricator links

[Conor Dooley]

On Tue, Jan 09, 2024 at 03:16:30PM -0700, Nathan Chancellor wrote:
> reviews.llvm.org was LLVM's Phabricator instances for code review. It
> has been abandoned in favor of GitHub pull requests. While the majority
> of links in the kernel sources still work because of the work Fangrui
> has done turning the dynamic Phabricator instance into a static archive,
> there are some issues with that work, so preemptively convert all the
> links in the kernel sources to point to the commit on GitHub.
> 
> Most of the commits have the corresponding differential review link in
> the commit message itself so there should not be any loss of fidelity in
> the relevant information.
> 
> Link: https://discourse.llvm.org/t/update-on-github-pull-requests/71540/172
> Signed-off-by: Nathan Chancellor 
> ---

>  arch/riscv/Kconfig  | 2 +-
>  arch/riscv/include/asm/ftrace.h | 2 +-

Reviewed-by: Conor Dooley 

Cheers,
Conor.


signature.asc
Description: PGP signature

Re: [PATCH 3/3] ASoC: dt-bindings: fsl,micfil: Add compatible string for i.MX95 platform

[Conor Dooley]

On Tue, Jan 09, 2024 at 04:55:51PM +0900, Chancel Liu wrote:
> Add compatible string "fsl,imx95-micfil" for i.MX95 platform.
> 
> Signed-off-by: Chancel Liu 
> ---
>  .../devicetree/bindings/sound/fsl,micfil.yaml | 15 +++
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/sound/fsl,micfil.yaml 
> b/Documentation/devicetree/bindings/sound/fsl,micfil.yaml
> index b7e605835639..f0d3d11d07d2 100644
> --- a/Documentation/devicetree/bindings/sound/fsl,micfil.yaml
> +++ b/Documentation/devicetree/bindings/sound/fsl,micfil.yaml
> @@ -15,10 +15,17 @@ description: |
>  
>  properties:
>compatible:
> -enum:
> -  - fsl,imx8mm-micfil
> -  - fsl,imx8mp-micfil
> -  - fsl,imx93-micfil
> +oneOf:
> +  - items:
> +  - enum:
> +  - fsl,imx95-micfil
> +  - const: fsl,imx93-micfil
> +

> +  - items:

This items is not needed, as the only item in the list is the enum.
You can just do
properties:
  compatible:
oneOf:
  - items:
  - enum:
  - fsl,imx95-micfil
  - const: fsl,imx93-micfil

  - enum:
  - fsl,imx8mm-micfil
  - fsl,imx8mp-micfil
  - fsl,imx93-micfil

Cheers,
Conor.

> +  - enum:
> +  - fsl,imx8mm-micfil
> +  - fsl,imx8mp-micfil
> +  - fsl,imx93-micfil
>  
>reg:
>  maxItems: 1
> -- 
> 2.42.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v4 5/7] kexec_file, ricv: print out debugging message if required

[Conor Dooley]

On Wed, Dec 13, 2023 at 01:57:45PM +0800, Baoquan He wrote:
> Then when specifying '-d' for kexec_file_load interface, loaded
> locations of kernel/initrd/cmdline etc can be printed out to help debug.
> 
> Here replace pr_debug() with the newly added kexec_dprintk() in kexec_file
> loading related codes.
> 
> And also replace pr_notice() with kexec_dprintk() in elf_kexec_load()
> because loaded location of purgatory and device tree are only printed
> out for debugging, it doesn't make sense to always print them out.
> 
> And also remove kexec_image_info() because the content has been printed
> out in generic code.
> 
> Signed-off-by: Baoquan He 

I'm sorry - I meant to look at this several days ago but I forgot.
Apart from the typo that crept back into $subject, this version explains
the rationale behind what you're changing a lot better, thanks.

Reviewed-by: Conor Dooley 

Cheers,
Conor.


signature.asc
Description: PGP signature

Re: [PATCH v3 5/7] kexec_file, ricv: print out debugging message if required

[Conor Dooley]

On Wed, Dec 06, 2023 at 11:37:52PM +0800, Baoquan He wrote:
> On 12/04/23 at 04:14pm, Conor Dooley wrote:
> > On Mon, Dec 04, 2023 at 11:38:05PM +0800, Baoquan He wrote:
> > > On 12/01/23 at 10:38am, Conor Dooley wrote:
> > > > On Thu, Nov 30, 2023 at 10:39:53AM +0800, Baoquan He wrote:
> > > > 
> > > > $subject has a typo in the arch bit :)
> > > 
> > > Indeed, will fix if need report. Thanks for careful checking.
> > > 
> > > > 
> > > > > Replace pr_debug() with the newly added kexec_dprintk() in kexec_file
> > > > > loading related codes.
> > > > 
> > > > Commit messages should be understandable in isolation, but this only
> > > > explains (part of) what is obvious in the diff. Why is this change
> > > > being made?
> > > 
> > > The purpose has been detailedly described in cover letter and patch 1
> > > log. Andrew has picked these patches into his tree and grabbed the cover
> > > letter log into the relevant commit for people's later checking. All
> > > these seven patches will be present in mainline together. This is common
> > > way when posting patch series? Please let me know if I misunderstand
> > > anything.
> > 
> > Each patch having a commit message that explains why a change is being
> > made is the expectation. It is especially useful to explain the why
> > here, since it is not just a mechanical conversion of pr_debug()s as the
> > commit message suggests.
> 
> Sounds reasonable. I rephrase the patch 3 log as below, do you think
> it's OK to you?

Yes, but with one comment.

> 
> I will also adjust patch logs on other ARCH once this one is done.
> Thanks.
> 
> =
> Subject: [PATCH v3 5/7] kexec_file, ricv: print out debugging message if 
> required
> 
> Then when specifying '-d' for kexec_file_load interface, loaded
> locations of kernel/initrd/cmdline etc can be printed out to help debug.
> 
> Here replace pr_debug() with the newly added kexec_dprintk() in kexec_file
> loading related codes.
> 

> And also replace pr_notice() with kexec_dprintk() in elf_kexec_load()
> because it's make sense to always print out loaded location of purgatory
> and device tree even though users don't expect the message.

This seems to contradict what you said in your earlier mail, about
moving these from notice to debug. I think you missed a negation in your
new version of the commit message. What you said in response to me seems
like a more complete explanation anyway:
always printing out the loaded location of purgatory and
device tree doesn't make sense. It will be confusing when users
see these even when they do normal kexec/kdump loading.

Thanks,
Conor.

> And also remove kexec_image_info() because the content has been printed
> out in generic code.
> 
> 
> 
> > 
> > > > 
> > > > > 
> > > > > And also remove kexec_image_info() because the content has been 
> > > > > printed
> > > > > out in generic code.
> > > > > 
> > > > > Signed-off-by: Baoquan He 
> > > > > ---
> > > > >  arch/riscv/kernel/elf_kexec.c | 11 ++-
> > > > >  arch/riscv/kernel/machine_kexec.c | 26 --
> > > > >  2 files changed, 6 insertions(+), 31 deletions(-)
> > > > > 
> > > > > diff --git a/arch/riscv/kernel/elf_kexec.c 
> > > > > b/arch/riscv/kernel/elf_kexec.c
> > > > > index e60fbd8660c4..5bd1ec3341fe 100644
> > > > > --- a/arch/riscv/kernel/elf_kexec.c
> > > > > +++ b/arch/riscv/kernel/elf_kexec.c
> > > > > @@ -216,7 +216,6 @@ static void *elf_kexec_load(struct kimage *image, 
> > > > > char *kernel_buf,
> > > > >   if (ret)
> > > > >   goto out;
> > > > >   kernel_start = image->start;
> > > > > - pr_notice("The entry point of kernel at 0x%lx\n", image->start);
> > > > >  
> > > > >   /* Add the kernel binary to the image */
> > > > >   ret = riscv_kexec_elf_load(image, &ehdr, &elf_info,
> > > > > @@ -252,8 +251,8 @@ static void *elf_kexec_load(struct kimage *image, 
> > > > > char *kernel_buf,
> > > > >   image->elf_load_addr = kbuf.mem;
> > > > >   image->elf_headers_sz = headers_sz;
> &

Re: [PATCH v3 5/7] kexec_file, ricv: print out debugging message if required

[Conor Dooley]

On Mon, Dec 04, 2023 at 11:38:05PM +0800, Baoquan He wrote:
> On 12/01/23 at 10:38am, Conor Dooley wrote:
> > On Thu, Nov 30, 2023 at 10:39:53AM +0800, Baoquan He wrote:
> > 
> > $subject has a typo in the arch bit :)
> 
> Indeed, will fix if need report. Thanks for careful checking.
> 
> > 
> > > Replace pr_debug() with the newly added kexec_dprintk() in kexec_file
> > > loading related codes.
> > 
> > Commit messages should be understandable in isolation, but this only
> > explains (part of) what is obvious in the diff. Why is this change
> > being made?
> 
> The purpose has been detailedly described in cover letter and patch 1
> log. Andrew has picked these patches into his tree and grabbed the cover
> letter log into the relevant commit for people's later checking. All
> these seven patches will be present in mainline together. This is common
> way when posting patch series? Please let me know if I misunderstand
> anything.

Each patch having a commit message that explains why a change is being
made is the expectation. It is especially useful to explain the why
here, since it is not just a mechanical conversion of pr_debug()s as the
commit message suggests.

> > 
> > > 
> > > And also remove kexec_image_info() because the content has been printed
> > > out in generic code.
> > > 
> > > Signed-off-by: Baoquan He 
> > > ---
> > >  arch/riscv/kernel/elf_kexec.c | 11 ++-
> > >  arch/riscv/kernel/machine_kexec.c | 26 --
> > >  2 files changed, 6 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/arch/riscv/kernel/elf_kexec.c b/arch/riscv/kernel/elf_kexec.c
> > > index e60fbd8660c4..5bd1ec3341fe 100644
> > > --- a/arch/riscv/kernel/elf_kexec.c
> > > +++ b/arch/riscv/kernel/elf_kexec.c
> > > @@ -216,7 +216,6 @@ static void *elf_kexec_load(struct kimage *image, 
> > > char *kernel_buf,
> > >   if (ret)
> > >   goto out;
> > >   kernel_start = image->start;
> > > - pr_notice("The entry point of kernel at 0x%lx\n", image->start);
> > >  
> > >   /* Add the kernel binary to the image */
> > >   ret = riscv_kexec_elf_load(image, &ehdr, &elf_info,
> > > @@ -252,8 +251,8 @@ static void *elf_kexec_load(struct kimage *image, 
> > > char *kernel_buf,
> > >   image->elf_load_addr = kbuf.mem;
> > >   image->elf_headers_sz = headers_sz;
> > >  
> > > - pr_debug("Loaded elf core header at 0x%lx bufsz=0x%lx 
> > > memsz=0x%lx\n",
> > > -  image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
> > > + kexec_dprintk("Loaded elf core header at 0x%lx bufsz=0x%lx 
> > > memsz=0x%lx\n",
> > > +   image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
> > >  
> > >   /* Setup cmdline for kdump kernel case */
> > >   modified_cmdline = setup_kdump_cmdline(image, cmdline,
> > > @@ -275,6 +274,8 @@ static void *elf_kexec_load(struct kimage *image, 
> > > char *kernel_buf,
> > >   pr_err("Error loading purgatory ret=%d\n", ret);
> > >   goto out;
> > >   }
> > > + kexec_dprintk("Loaded purgatory at 0x%lx\n", kbuf.mem);
> > > +
> > >   ret = kexec_purgatory_get_set_symbol(image, "riscv_kernel_entry",
> > >&kernel_start,
> > >sizeof(kernel_start), 0);
> > > @@ -293,7 +294,7 @@ static void *elf_kexec_load(struct kimage *image, 
> > > char *kernel_buf,
> > >   if (ret)
> > >   goto out;
> > >   initrd_pbase = kbuf.mem;
> > 
> > > - pr_notice("Loaded initrd at 0x%lx\n", initrd_pbase);
> > > + kexec_dprintk("Loaded initrd at 0x%lx\n", initrd_pbase);
> > 
> > This is not a pr_debug().
> > 
> > >   }
> > >  
> > >   /* Add the DTB to the image */
> > > @@ -318,7 +319,7 @@ static void *elf_kexec_load(struct kimage *image, 
> > > char *kernel_buf,
> > >   }
> > >   /* Cache the fdt buffer address for memory cleanup */
> > >   image->arch.fdt = fdt;
> > 
> > > - pr_notice("Loaded device tree at 0x%lx\n", kbuf.mem);
> > > + kexec_dprintk("Loaded device tree at 0x%lx\n", kbuf.mem);
> > 
> > Neither is this. Why are they being moved from pr_notice()?
> 
> You are right. 
> 
> While always printing out the loaded location of purgatory and
> device tree doesn't make sense. It will be confusing when users
> see these even when they do normal kexec/kdump loading. It should be
> changed to pr_debug().
> 
> Which way do you suggest?
> 1) change it back to pr_debug(), fix it in another patch;
> 2) keep it as is in the patch;

Personally I think it is fine to change them all in one patch, but the
rationale for converting pr_notice() to your new debug infrastructure
needs to be in the commit message.

Thanks,
Conor.


signature.asc
Description: PGP signature

Re: [PATCH v3 5/7] kexec_file, ricv: print out debugging message if required

[Conor Dooley]

On Thu, Nov 30, 2023 at 10:39:53AM +0800, Baoquan He wrote:

$subject has a typo in the arch bit :)

> Replace pr_debug() with the newly added kexec_dprintk() in kexec_file
> loading related codes.

Commit messages should be understandable in isolation, but this only
explains (part of) what is obvious in the diff. Why is this change
being made?

> 
> And also remove kexec_image_info() because the content has been printed
> out in generic code.
> 
> Signed-off-by: Baoquan He 
> ---
>  arch/riscv/kernel/elf_kexec.c | 11 ++-
>  arch/riscv/kernel/machine_kexec.c | 26 --
>  2 files changed, 6 insertions(+), 31 deletions(-)
> 
> diff --git a/arch/riscv/kernel/elf_kexec.c b/arch/riscv/kernel/elf_kexec.c
> index e60fbd8660c4..5bd1ec3341fe 100644
> --- a/arch/riscv/kernel/elf_kexec.c
> +++ b/arch/riscv/kernel/elf_kexec.c
> @@ -216,7 +216,6 @@ static void *elf_kexec_load(struct kimage *image, char 
> *kernel_buf,
>   if (ret)
>   goto out;
>   kernel_start = image->start;
> - pr_notice("The entry point of kernel at 0x%lx\n", image->start);
>  
>   /* Add the kernel binary to the image */
>   ret = riscv_kexec_elf_load(image, &ehdr, &elf_info,
> @@ -252,8 +251,8 @@ static void *elf_kexec_load(struct kimage *image, char 
> *kernel_buf,
>   image->elf_load_addr = kbuf.mem;
>   image->elf_headers_sz = headers_sz;
>  
> - pr_debug("Loaded elf core header at 0x%lx bufsz=0x%lx 
> memsz=0x%lx\n",
> -  image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
> + kexec_dprintk("Loaded elf core header at 0x%lx bufsz=0x%lx 
> memsz=0x%lx\n",
> +   image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
>  
>   /* Setup cmdline for kdump kernel case */
>   modified_cmdline = setup_kdump_cmdline(image, cmdline,
> @@ -275,6 +274,8 @@ static void *elf_kexec_load(struct kimage *image, char 
> *kernel_buf,
>   pr_err("Error loading purgatory ret=%d\n", ret);
>   goto out;
>   }
> + kexec_dprintk("Loaded purgatory at 0x%lx\n", kbuf.mem);
> +
>   ret = kexec_purgatory_get_set_symbol(image, "riscv_kernel_entry",
>&kernel_start,
>sizeof(kernel_start), 0);
> @@ -293,7 +294,7 @@ static void *elf_kexec_load(struct kimage *image, char 
> *kernel_buf,
>   if (ret)
>   goto out;
>   initrd_pbase = kbuf.mem;

> - pr_notice("Loaded initrd at 0x%lx\n", initrd_pbase);
> + kexec_dprintk("Loaded initrd at 0x%lx\n", initrd_pbase);

This is not a pr_debug().

>   }
>  
>   /* Add the DTB to the image */
> @@ -318,7 +319,7 @@ static void *elf_kexec_load(struct kimage *image, char 
> *kernel_buf,
>   }
>   /* Cache the fdt buffer address for memory cleanup */
>   image->arch.fdt = fdt;

> - pr_notice("Loaded device tree at 0x%lx\n", kbuf.mem);
> + kexec_dprintk("Loaded device tree at 0x%lx\n", kbuf.mem);

Neither is this. Why are they being moved from pr_notice()?

Thanks,
Conor.

>   goto out;
>  
>  out_free_fdt:
> diff --git a/arch/riscv/kernel/machine_kexec.c 
> b/arch/riscv/kernel/machine_kexec.c
> index 2d139b724bc8..ed9cad20c039 100644
> --- a/arch/riscv/kernel/machine_kexec.c
> +++ b/arch/riscv/kernel/machine_kexec.c
> @@ -18,30 +18,6 @@
>  #include 
>  #include 
>  
> -/*
> - * kexec_image_info - Print received image details
> - */
> -static void
> -kexec_image_info(const struct kimage *image)
> -{
> - unsigned long i;
> -
> - pr_debug("Kexec image info:\n");
> - pr_debug("\ttype:%d\n", image->type);
> - pr_debug("\tstart:   %lx\n", image->start);
> - pr_debug("\thead:%lx\n", image->head);
> - pr_debug("\tnr_segments: %lu\n", image->nr_segments);
> -
> - for (i = 0; i < image->nr_segments; i++) {
> - pr_debug("\tsegment[%lu]: %016lx - %016lx", i,
> - image->segment[i].mem,
> - image->segment[i].mem + image->segment[i].memsz);
> - pr_debug("\t\t0x%lx bytes, %lu pages\n",
> - (unsigned long) image->segment[i].memsz,
> - (unsigned long) image->segment[i].memsz /  PAGE_SIZE);
> - }
> -}
> -
>  /*
>   * machine_kexec_prepare - Initialize kexec
>   *
> @@ -60,8 +36,6 @@ machine_kexec_prepare(struct kimage *image)
>   unsigned int control_code_buffer_sz = 0;
>   int i = 0;
>  
> - kexec_image_info(image);
> -
>   /* Find the Flattened Device Tree and save its physical address */
>   for (i = 0; i < image->nr_segments; i++) {
>   if (image->segment[i].memsz <= sizeof(fdt))
> -- 
> 2.41.0
> 


signature.asc
Description: PGP signature

Re: [PATCH 1/2] kexec: fix KEXEC_FILE dependencies

[Conor Dooley]

On Mon, Oct 23, 2023 at 06:04:06PM +0200, Arnd Bergmann wrote:
> On Mon, Oct 23, 2023, at 17:37, Conor Dooley wrote:
> > On Mon, Oct 23, 2023 at 01:01:54PM +0200, Arnd Bergmann wrote:
> 
> >> index 25474f8c12b79..f571bad2d22d0 100644
> >> --- a/arch/riscv/Kconfig
> >> +++ b/arch/riscv/Kconfig
> >> @@ -687,9 +687,7 @@ config ARCH_SELECTS_KEXEC_FILE
> >>select KEXEC_ELF
> >>  
> >>  config ARCH_SUPPORTS_KEXEC_PURGATORY
> >> -  def_bool KEXEC_FILE
> >> -  depends on CRYPTO=y
> >> -  depends on CRYPTO_SHA256=y
> >> +  def_bool y
> >
> > This being the problem, KEXEC_FILE is 64-bit only.
> >
> > IIRC I commented on this same thing during the original conversion
> > patches.
> 
> Does it work with this patch on top?

Yah, that modification builds.

rv32 being the redheaded stepchild strikes again :!

Cheers,
Conor.

> 
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -687,7 +687,7 @@ config ARCH_SELECTS_KEXEC_FILE
> select KEXEC_ELF
>  
>  config ARCH_SUPPORTS_KEXEC_PURGATORY
> -   def_bool y
> +   def_bool ARCH_SUPPORTS_KEXEC_FILE
>  
>  config ARCH_SUPPORTS_CRASH_DUMP
> def_bool y
> 
> 
>  Arnd


signature.asc
Description: PGP signature

Re: [PATCH 1/2] kexec: fix KEXEC_FILE dependencies

[Conor Dooley]

On Mon, Oct 23, 2023 at 01:01:54PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann 
> 
> The cleanup for the CONFIG_KEXEC Kconfig logic accidentally changed the
> 'depends on CRYPTO=y' dependency to a plain 'depends on CRYPTO', which
> causes a link failure when all the crypto support is in a loadable module
> and kexec_file support is built-in:
> 
> x86_64-linux-ld: vmlinux.o: in function `__x64_sys_kexec_file_load':
> (.text+0x32e30a): undefined reference to `crypto_alloc_shash'
> x86_64-linux-ld: (.text+0x32e58e): undefined reference to 
> `crypto_shash_update'
> x86_64-linux-ld: (.text+0x32e6ee): undefined reference to `crypto_shash_final'
> 
> Both s390 and x86 have this problem, while ppc64 and riscv have the
> correct dependency already. On riscv, the dependency is only used
> for the purgatory, not for the kexec_file code itself, which may
> be a bit surprising as it means that with CONFIG_CRYPTO=m, it is
> possible to enable KEXEC_FILE but then the purgatory code is silently
> left out.
> 
> Move this into the common Kconfig.kexec file in a way that is
> correct everywhere, using the dependency on CRYPTO_SHA256=y only
> when the purgatory code is available. This requires reversing the
> dependency between ARCH_SUPPORTS_KEXEC_PURGATORY and KEXEC_FILE,
> but the effect remains the same, other than making riscv behave
> like the other ones.
> 
> On s390, there is an additional dependency on CRYPTO_SHA256_S390, which
> should technically not be required but gives better performance. Remove
> this dependency here, noting that it was not present in the initial
> Kconfig code but was brought in without an explanation in commit
> 71406883fd357 ("s390/kexec_file: Add kexec_file_load system call").
> 
> Fixes: 6af5138083005 ("x86/kexec: refactor for kernel/Kconfig.kexec")
> Signed-off-by: Arnd Bergmann 
> ---
>  arch/powerpc/Kconfig | 4 ++--

>  arch/riscv/Kconfig   | 4 +---

This doesn't appear to work for rv32. The defconfig build in our
patchwork automation complains:
  /tmp/tmp.Aq21JVRQTx/arch/riscv/purgatory/entry.S:29:2: error: instruction 
requires the following: RV64I Base Instruction Set

>  arch/s390/Kconfig| 4 ++--
>  arch/x86/Kconfig | 4 ++--
>  kernel/Kconfig.kexec | 1 +
>  5 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index d5d5388973ac7..4640cee33f123 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -607,10 +607,10 @@ config ARCH_SUPPORTS_KEXEC
>   def_bool PPC_BOOK3S || PPC_E500 || (44x && !SMP)
>  
>  config ARCH_SUPPORTS_KEXEC_FILE
> - def_bool PPC64 && CRYPTO=y && CRYPTO_SHA256=y
> + def_bool PPC64
>  
>  config ARCH_SUPPORTS_KEXEC_PURGATORY
> - def_bool KEXEC_FILE
> + def_bool y
>  
>  config ARCH_SELECTS_KEXEC_FILE
>   def_bool y
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 25474f8c12b79..f571bad2d22d0 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -687,9 +687,7 @@ config ARCH_SELECTS_KEXEC_FILE
>   select KEXEC_ELF
>  
>  config ARCH_SUPPORTS_KEXEC_PURGATORY
> - def_bool KEXEC_FILE
> - depends on CRYPTO=y
> - depends on CRYPTO_SHA256=y
> + def_bool y

This being the problem, KEXEC_FILE is 64-bit only.

IIRC I commented on this same thing during the original conversion
patches.

Cheers,
Conor.

>  
>  config ARCH_SUPPORTS_CRASH_DUMP
>   def_bool y
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index b0d67ac8695f9..ec77106af4137 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -253,13 +253,13 @@ config ARCH_SUPPORTS_KEXEC
>   def_bool y
>  
>  config ARCH_SUPPORTS_KEXEC_FILE
> - def_bool CRYPTO && CRYPTO_SHA256 && CRYPTO_SHA256_S390
> + def_bool y
>  
>  config ARCH_SUPPORTS_KEXEC_SIG
>   def_bool MODULE_SIG_FORMAT
>  
>  config ARCH_SUPPORTS_KEXEC_PURGATORY
> - def_bool KEXEC_FILE
> + def_bool y
>  
>  config ARCH_SUPPORTS_CRASH_DUMP
>   def_bool y
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 94efde80ebf35..f9975b15ccd57 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2073,7 +2073,7 @@ config ARCH_SUPPORTS_KEXEC
>   def_bool y
>  
>  config ARCH_SUPPORTS_KEXEC_FILE
> - def_bool X86_64 && CRYPTO && CRYPTO_SHA256
> + def_bool X86_64
>  
>  config ARCH_SELECTS_KEXEC_FILE
>   def_bool y
> @@ -2081,7 +2081,7 @@ config ARCH_SELECTS_KEXEC_FILE
>   select HAVE_IMA_KEXEC if IMA
>  
>  config ARCH_SUPPORTS_KEXEC_PURGATORY
> - def_bool KEXEC_FILE
> + def_bool y
>  
>  config ARCH_SUPPORTS_KEXEC_SIG
>   def_bool y
> diff --git a/kernel/Kconfig.kexec b/kernel/Kconfig.kexec
> index 7aff28ded2f48..bfc636d64ff2b 100644
> --- a/kernel/Kconfig.kexec
> +++ b/kernel/Kconfig.kexec
> @@ -36,6 +36,7 @@ config KEXEC
>  config KEXEC_FILE
>   bool "Enable kexec file based system call"
>   depends on ARCH_SUPPORTS_KEXEC_FILE
> + depends on CRYPTO_SHA256=y || !ARCH_SUPPORTS_KEXEC_PURGATORY
>   select KEXEC_CO

Re: [PATCH] ASoC: dt-bindings: Add missing (unevaluated|additional)Properties on child node schemas

[Conor Dooley]

On Mon, Sep 25, 2023 at 05:09:28PM -0500, Rob Herring wrote:
> Just as unevaluatedProperties or additionalProperties are required at
> the top level of schemas, they should (and will) also be required for
> child node schemas. That ensures only documented properties are
> present for any node.
> 
> Add unevaluatedProperties or additionalProperties as appropriate.
> 
> Signed-off-by: Rob Herring 

Acked-by: Conor Dooley 

Thanks,
Conor.

> ---
>  Documentation/devicetree/bindings/sound/dialog,da7219.yaml | 1 +
>  Documentation/devicetree/bindings/sound/fsl,qmc-audio.yaml | 1 +
>  Documentation/devicetree/bindings/sound/ti,pcm3168a.yaml   | 1 +
>  3 files changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/dialog,da7219.yaml 
> b/Documentation/devicetree/bindings/sound/dialog,da7219.yaml
> index eb7d219e2c86..19137abdba3e 100644
> --- a/Documentation/devicetree/bindings/sound/dialog,da7219.yaml
> +++ b/Documentation/devicetree/bindings/sound/dialog,da7219.yaml
> @@ -89,6 +89,7 @@ properties:
>  
>da7219_aad:
>  type: object
> +additionalProperties: false
>  description:
>Configuration of advanced accessory detection.
>  properties:
> diff --git a/Documentation/devicetree/bindings/sound/fsl,qmc-audio.yaml 
> b/Documentation/devicetree/bindings/sound/fsl,qmc-audio.yaml
> index ff5cd9241941..b522ed7dcc51 100644
> --- a/Documentation/devicetree/bindings/sound/fsl,qmc-audio.yaml
> +++ b/Documentation/devicetree/bindings/sound/fsl,qmc-audio.yaml
> @@ -33,6 +33,7 @@ patternProperties:
>  description:
>A DAI managed by this controller
>  type: object
> +additionalProperties: false
>  
>  properties:
>reg:
> diff --git a/Documentation/devicetree/bindings/sound/ti,pcm3168a.yaml 
> b/Documentation/devicetree/bindings/sound/ti,pcm3168a.yaml
> index b6a4360ab845..0b4f003989a4 100644
> --- a/Documentation/devicetree/bindings/sound/ti,pcm3168a.yaml
> +++ b/Documentation/devicetree/bindings/sound/ti,pcm3168a.yaml
> @@ -60,6 +60,7 @@ properties:
>  
>ports:
>  $ref: audio-graph-port.yaml#/definitions/port-base
> +unevaluatedProperties: false
>  properties:
>port@0:
>  $ref: audio-graph-port.yaml#
> -- 
> 2.40.1
> 


signature.asc
Description: PGP signature

Re: [PATCH v5 08/31] dt-bindings: soc: fsl: cpm_qe: cpm1-scc-qmc: Add support for QMC HDLC

[Conor Dooley]

On Wed, Sep 13, 2023 at 03:56:16PM +0100, Conor Dooley wrote:
> On Wed, Sep 13, 2023 at 04:52:50PM +0200, Herve Codina wrote:
> > On Wed, 13 Sep 2023 15:42:45 +0100
> > Conor Dooley  wrote:
> > 
> > > On Wed, Sep 13, 2023 at 09:26:40AM +0200, Herve Codina wrote:
> > > > Hi Conor,
> > > > 
> > > > On Tue, 12 Sep 2023 18:21:58 +0100
> > > > Conor Dooley  wrote:
> > > >   
> > > > > On Tue, Sep 12, 2023 at 12:10:18PM +0200, Herve Codina wrote:  
> > > > > > The QMC (QUICC mutichannel controller) is a controller present in 
> > > > > > some
> > > > > > PowerQUICC SoC such as MPC885.
> > > > > > The QMC HDLC uses the QMC controller to transfer HDLC data.
> > > > > > 
> > > > > > Additionally, a framer can be connected to the QMC HDLC.
> > > > > > If present, this framer is the interface between the TDM bus used 
> > > > > > by the
> > > > > > QMC HDLC and the E1/T1 line.
> > > > > > The QMC HDLC can use this framer to get information about the E1/T1 
> > > > > > line
> > > > > > and configure the E1/T1 line.
> > > > > > 
> > > > > > Signed-off-by: Herve Codina 
> > > > > > ---
> > > > > >  .../bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml   | 13 
> > > > > > +
> > > > > >  1 file changed, 13 insertions(+)
> > > > > > 
> > > > > > diff --git 
> > > > > > a/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml
> > > > > >  
> > > > > > b/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml
> > > > > > index 82d9beb48e00..b5073531f3f1 100644
> > > > > > --- 
> > > > > > a/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml
> > > > > > +++ 
> > > > > > b/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml
> > > > > > @@ -101,6 +101,16 @@ patternProperties:
> > > > > >Channel assigned Rx time-slots within the Rx time-slots 
> > > > > > routed by the
> > > > > >TSA to this cell.
> > > > > >  
> > > > > > +  compatible:
> > > > > > +const: fsl,qmc-hdlc
> > > > > > +
> > > > > > +  fsl,framer:
> > > > > > +$ref: /schemas/types.yaml#/definitions/phandle
> > > > > > +description:
> > > > > > +  phandle to the framer node. The framer is in charge of 
> > > > > > an E1/T1 line
> > > > > > +  interface connected to the TDM bus. It can be used to 
> > > > > > get the E1/T1 line
> > > > > > +  status such as link up/down.
> > > > > 
> > > > > Sounds like this fsl,framer property should depend on the compatible
> > > > > being present, no?  
> > > > 
> > > > Well from the implementation point of view, only the QMC HDLC driver 
> > > > uses this
> > > > property.
> > > > 
> > > > From the hardware description point of view, this property means that 
> > > > the time slots
> > > > handled by this channel are connected to the framer. So I think it 
> > > > makes sense for
> > > > any channel no matter the compatible (even if compatible is not 
> > > > present).
> > > > 
> > > > Should I change and constraint the fsl,framer property to the 
> > > > compatible presence ?
> > > > If so, is the following correct for this contraint ?
> > > >--- 8< ---
> > > >dependencies:
> > > >  - fsl,framer: [ compatible ];
> > > >--- 8< ---  
> > > 
> > > The regular sort of
> > > if:
> > >   compatible:
> > >   contains:
> > >   const: foo
> > > then:
> > >   required:
> > >   - fsl,framer
> > > would fit the bill, no?
> > 
> > Not sure.
> > "fsl,framer" is an optional property (depending on the hardware we can have
> > a framer or not).
> 
> Ah apologies, I had it backwards! Your suggestion seems fair in that
> case.

Or actually,
if:
compatible:
not:
contains:
const: foo
 then:
properties:
fsl,framer: false
? That should do the trick in a more conventional way.


signature.asc
Description: PGP signature

Re: [PATCH v5 08/31] dt-bindings: soc: fsl: cpm_qe: cpm1-scc-qmc: Add support for QMC HDLC

[Conor Dooley]

On Wed, Sep 13, 2023 at 04:52:50PM +0200, Herve Codina wrote:
> On Wed, 13 Sep 2023 15:42:45 +0100
> Conor Dooley  wrote:
> 
> > On Wed, Sep 13, 2023 at 09:26:40AM +0200, Herve Codina wrote:
> > > Hi Conor,
> > > 
> > > On Tue, 12 Sep 2023 18:21:58 +0100
> > > Conor Dooley  wrote:
> > >   
> > > > On Tue, Sep 12, 2023 at 12:10:18PM +0200, Herve Codina wrote:  
> > > > > The QMC (QUICC mutichannel controller) is a controller present in some
> > > > > PowerQUICC SoC such as MPC885.
> > > > > The QMC HDLC uses the QMC controller to transfer HDLC data.
> > > > > 
> > > > > Additionally, a framer can be connected to the QMC HDLC.
> > > > > If present, this framer is the interface between the TDM bus used by 
> > > > > the
> > > > > QMC HDLC and the E1/T1 line.
> > > > > The QMC HDLC can use this framer to get information about the E1/T1 
> > > > > line
> > > > > and configure the E1/T1 line.
> > > > > 
> > > > > Signed-off-by: Herve Codina 
> > > > > ---
> > > > >  .../bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml   | 13 
> > > > > +
> > > > >  1 file changed, 13 insertions(+)
> > > > > 
> > > > > diff --git 
> > > > > a/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml
> > > > >  
> > > > > b/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml
> > > > > index 82d9beb48e00..b5073531f3f1 100644
> > > > > --- 
> > > > > a/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml
> > > > > +++ 
> > > > > b/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml
> > > > > @@ -101,6 +101,16 @@ patternProperties:
> > > > >Channel assigned Rx time-slots within the Rx time-slots 
> > > > > routed by the
> > > > >TSA to this cell.
> > > > >  
> > > > > +  compatible:
> > > > > +const: fsl,qmc-hdlc
> > > > > +
> > > > > +  fsl,framer:
> > > > > +$ref: /schemas/types.yaml#/definitions/phandle
> > > > > +description:
> > > > > +  phandle to the framer node. The framer is in charge of an 
> > > > > E1/T1 line
> > > > > +  interface connected to the TDM bus. It can be used to get 
> > > > > the E1/T1 line
> > > > > +  status such as link up/down.
> > > > 
> > > > Sounds like this fsl,framer property should depend on the compatible
> > > > being present, no?  
> > > 
> > > Well from the implementation point of view, only the QMC HDLC driver uses 
> > > this
> > > property.
> > > 
> > > From the hardware description point of view, this property means that the 
> > > time slots
> > > handled by this channel are connected to the framer. So I think it makes 
> > > sense for
> > > any channel no matter the compatible (even if compatible is not present).
> > > 
> > > Should I change and constraint the fsl,framer property to the compatible 
> > > presence ?
> > > If so, is the following correct for this contraint ?
> > >--- 8< ---
> > >dependencies:
> > >  - fsl,framer: [ compatible ];
> > >--- 8< ---  
> > 
> > The regular sort of
> > if:
> > compatible:
> > contains:
> > const: foo
> > then:
> > required:
> > - fsl,framer
> > would fit the bill, no?
> 
> Not sure.
> "fsl,framer" is an optional property (depending on the hardware we can have
> a framer or not).

Ah apologies, I had it backwards! Your suggestion seems fair in that
case.

Thanks,
Conor.


signature.asc
Description: PGP signature

Re: [PATCH v5 08/31] dt-bindings: soc: fsl: cpm_qe: cpm1-scc-qmc: Add support for QMC HDLC

[Conor Dooley]

On Wed, Sep 13, 2023 at 09:26:40AM +0200, Herve Codina wrote:
> Hi Conor,
> 
> On Tue, 12 Sep 2023 18:21:58 +0100
> Conor Dooley  wrote:
> 
> > On Tue, Sep 12, 2023 at 12:10:18PM +0200, Herve Codina wrote:
> > > The QMC (QUICC mutichannel controller) is a controller present in some
> > > PowerQUICC SoC such as MPC885.
> > > The QMC HDLC uses the QMC controller to transfer HDLC data.
> > > 
> > > Additionally, a framer can be connected to the QMC HDLC.
> > > If present, this framer is the interface between the TDM bus used by the
> > > QMC HDLC and the E1/T1 line.
> > > The QMC HDLC can use this framer to get information about the E1/T1 line
> > > and configure the E1/T1 line.
> > > 
> > > Signed-off-by: Herve Codina 
> > > ---
> > >  .../bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml   | 13 +
> > >  1 file changed, 13 insertions(+)
> > > 
> > > diff --git 
> > > a/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml 
> > > b/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml
> > > index 82d9beb48e00..b5073531f3f1 100644
> > > --- 
> > > a/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml
> > > +++ 
> > > b/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml
> > > @@ -101,6 +101,16 @@ patternProperties:
> > >Channel assigned Rx time-slots within the Rx time-slots routed 
> > > by the
> > >TSA to this cell.
> > >  
> > > +  compatible:
> > > +const: fsl,qmc-hdlc
> > > +
> > > +  fsl,framer:
> > > +$ref: /schemas/types.yaml#/definitions/phandle
> > > +description:
> > > +  phandle to the framer node. The framer is in charge of an 
> > > E1/T1 line
> > > +  interface connected to the TDM bus. It can be used to get the 
> > > E1/T1 line
> > > +  status such as link up/down.  
> > 
> > Sounds like this fsl,framer property should depend on the compatible
> > being present, no?
> 
> Well from the implementation point of view, only the QMC HDLC driver uses this
> property.
> 
> From the hardware description point of view, this property means that the 
> time slots
> handled by this channel are connected to the framer. So I think it makes 
> sense for
> any channel no matter the compatible (even if compatible is not present).
> 
> Should I change and constraint the fsl,framer property to the compatible 
> presence ?
> If so, is the following correct for this contraint ?
>--- 8< ---
>dependencies:
>  - fsl,framer: [ compatible ];
>--- 8< ---

The regular sort of
if:
compatible:
contains:
const: foo
then:
required:
- fsl,framer
would fit the bill, no?


signature.asc
Description: PGP signature

Re: [PATCH v5 25/31] dt-bindings: net: Add the Lantiq PEF2256 E1/T1/J1 framer

[Conor Dooley]

On Tue, Sep 12, 2023 at 01:54:05PM -0500, Rob Herring wrote:
> > > +  lantiq,data-rate-bps:
> > > +$ref: /schemas/types.yaml#/definitions/uint32
> > > +enum: [2048000, 4096000, 8192000, 16384000]
> > 
> > -kBps is a standard suffix, would it be worth using that instead here?
> > What you have would fit as even multiples.
> > Otherwise Rob, should dt-schema grow -bps as a standard suffix?
> 
> Yeah, I think that makes sense. I've added it now.

Cool, thanks!


signature.asc
Description: PGP signature

Re: [PATCH v5 25/31] dt-bindings: net: Add the Lantiq PEF2256 E1/T1/J1 framer

[Conor Dooley]

Yo,

I'm not au fait enough with this to leave particularly meaningful
comments, so just some minor ones for you.

On Tue, Sep 12, 2023 at 12:14:44PM +0200, Herve Codina wrote:
> The Lantiq PEF2256 is a framer and line interface component designed to
> fulfill all required interfacing between an analog E1/T1/J1 line and the
> digital PCM system highway/H.100 bus.
> 
> Signed-off-by: Herve Codina 
> Signed-off-by: Christophe Leroy 

Missing a co-developed-by?

> ---
>  .../bindings/net/lantiq,pef2256.yaml  | 214 ++
>  1 file changed, 214 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/lantiq,pef2256.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/lantiq,pef2256.yaml 
> b/Documentation/devicetree/bindings/net/lantiq,pef2256.yaml
> new file mode 100644
> index ..c4f21678bf6a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/lantiq,pef2256.yaml
> @@ -0,0 +1,214 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/lantiq,pef2256.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Lantiq PEF2256
> +
> +maintainers:
> +  - Herve Codina 
> +
> +description:
> +  The Lantiq PEF2256, also known as Infineon PEF2256 or FALC56, is a framer 
> and
> +  line interface component designed to fulfill all required interfacing 
> between
> +  an analog E1/T1/J1 line and the digital PCM system highway/H.100 bus.
> +
> +properties:
> +  compatible:
> +items:
> +  - const: lantiq,pef2256
> +
> +  reg:
> +maxItems: 1
> +
> +  clocks:
> +items:
> +  - description: Master clock

My OCD is rather upset by the inconsistent capitalisation used here :/

> +  - description: System Clock Receive
> +  - description: System Clock Transmit
> +
> +  clock-names:
> +items:
> +  - const: mclk
> +  - const: sclkr
> +  - const: sclkx
> +
> +  interrupts:
> +maxItems: 1
> +
> +  reset-gpios:
> +description:
> +  GPIO used to reset the device.
> +maxItems: 1
> +
> +  pinctrl:
> +$ref: /schemas/pinctrl/pinctrl.yaml#
> +additionalProperties: false
> +
> +patternProperties:
> +  '-pins$':
> +type: object
> +$ref: /schemas/pinctrl/pinmux-node.yaml#
> +additionalProperties: false
> +
> +properties:
> +  pins:
> +enum: [ RPA, RPB, RPC, RPD, XPA, XPB, XPC, XPD ]
> +
> +  function:
> +enum: [ SYPR, RFM, RFMB, RSIGM, RSIG, DLR, FREEZE, RFSP, LOS,
> +SYPX, XFMS, XSIG, TCLK, XMFB, XSIGM, DLX, XCLK, XLT,
> +GPI, GPOH, GPOL ]
> +
> +required:
> +  - pins
> +  - function
> +
> +  lantiq,data-rate-bps:
> +$ref: /schemas/types.yaml#/definitions/uint32
> +enum: [2048000, 4096000, 8192000, 16384000]

-kBps is a standard suffix, would it be worth using that instead here?
What you have would fit as even multiples.
Otherwise Rob, should dt-schema grow -bps as a standard suffix?

> +default: 2048000
> +description:
> +  Data rate (bit per seconds) on the system highway.
> +
> +  lantiq,clock-falling-edge:
> +$ref: /schemas/types.yaml#/definitions/flag
> +description:
> +  Data is sent on falling edge of the clock (and received on the rising
> +  edge). If 'clock-falling-edge' is not present, data is sent on the
> +  rising edge (and received on the falling edge).
> +
> +  lantiq,channel-phase:
> +$ref: /schemas/types.yaml#/definitions/uint32
> +enum: [0, 1, 2, 3, 4, 5, 6, 7]
> +default: 0
> +description: |
> +  The pef2256 delivers a full frame (32 8bit time-slots in E1 and 24 8bit

Just a wee nit, s/8bit/8-bit/ :)

Rest of this I don't really feel like I can really review.

Thanks,
Conor.

> +  time-slots 8 8bit signaling in E1/J1) every 125us. This lead to a data
> +  rate of 2048000 bit/s. When lantiq,data-rate-bps is more than 2048000
> +  bit/s, the data (all 32 8bit) present in the frame are interleave with
> +  unused time-slots. The lantiq,channel-phase property allows to set the
> +  correct alignment of the interleave mechanism.
> +  For instance, suppose lantiq,data-rate-bps = 8192000 (ie 4*2048000), 
> and
> +  lantiq,channel-phase = 2, the interleave schema with unused time-slots
> +  (nu) and used time-slots (XX) for TSi is
> +nu nu XX nu nu nu XX nu nu nu XX nu
> +<-- TSi --> <- TSi+1 -> <- TSi+2 ->
> +  With lantiq,data-rate-bps = 8192000, and lantiq,channel-phase = 1, the
> +  interleave schema is
> +nu XX nu nu nu XX nu nu nu XX nu nu
> +<-- TSi --> <- TSi+1 -> <- TSi+2 ->
> +  With lantiq,data-rate-bps = 4096000 (ie 2*2048000), and
> +  lantiq,channel-phase = 1, the interleave schema is
> +nuXXnuXXnuXX
> +<-- TSi --> <- TSi+1 -> <- TSi+2 ->
> +
> +patternProperties:
> +  '^codec(-([0-9]|

Re: [PATCH v5 08/31] dt-bindings: soc: fsl: cpm_qe: cpm1-scc-qmc: Add support for QMC HDLC

[Conor Dooley]

On Tue, Sep 12, 2023 at 12:10:18PM +0200, Herve Codina wrote:
> The QMC (QUICC mutichannel controller) is a controller present in some
> PowerQUICC SoC such as MPC885.
> The QMC HDLC uses the QMC controller to transfer HDLC data.
> 
> Additionally, a framer can be connected to the QMC HDLC.
> If present, this framer is the interface between the TDM bus used by the
> QMC HDLC and the E1/T1 line.
> The QMC HDLC can use this framer to get information about the E1/T1 line
> and configure the E1/T1 line.
> 
> Signed-off-by: Herve Codina 
> ---
>  .../bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml   | 13 +
>  1 file changed, 13 insertions(+)
> 
> diff --git 
> a/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml 
> b/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml
> index 82d9beb48e00..b5073531f3f1 100644
> --- a/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml
> +++ b/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml
> @@ -101,6 +101,16 @@ patternProperties:
>Channel assigned Rx time-slots within the Rx time-slots routed by 
> the
>TSA to this cell.
>  
> +  compatible:
> +const: fsl,qmc-hdlc
> +
> +  fsl,framer:
> +$ref: /schemas/types.yaml#/definitions/phandle
> +description:
> +  phandle to the framer node. The framer is in charge of an E1/T1 
> line
> +  interface connected to the TDM bus. It can be used to get the 
> E1/T1 line
> +  status such as link up/down.

Sounds like this fsl,framer property should depend on the compatible
being present, no?

Thanks,
Conor.

> +
>  required:
>- reg
>- fsl,tx-ts-mask
> @@ -159,5 +169,8 @@ examples:
>  fsl,operational-mode = "hdlc";
>  fsl,tx-ts-mask = <0x 0xff00>;
>  fsl,rx-ts-mask = <0x 0xff00>;
> +
> +compatible = "fsl,qmc-hdlc";
> +fsl,framer = <&framer>;
>  };
>  };
> -- 
> 2.41.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v5 07/31] dt-bindings: soc: fsl: cpm_qe: cpm1-scc-qmc: Add 'additionalProperties: false' in child nodes

[Conor Dooley]

On Tue, Sep 12, 2023 at 10:14:58AM +0200, Herve Codina wrote:
> Additional properties in child node should not be allowed.
> 
> Prevent them adding 'additionalProperties: false'
> 
> Signed-off-by: Herve Codina 

Acked-by: Conor Dooley 

Thanks,
Conor.

> ---
>  .../devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git 
> a/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml 
> b/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml
> index 450a0354cb1d..82d9beb48e00 100644
> --- a/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml
> +++ b/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml
> @@ -64,6 +64,7 @@ patternProperties:
>  description:
>A channel managed by this controller
>  type: object
> +additionalProperties: false
>  
>  properties:
>reg:
> -- 
> 2.41.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v5 06/31] dt-bindings: soc: fsl: cpm_qe: cpm1-scc-qmc: Fix example property name

[Conor Dooley]

On Tue, Sep 12, 2023 at 10:14:57AM +0200, Herve Codina wrote:
> The given example mentions the 'fsl,mode' property whereas the
> correct property name, the one described, is 'fsl,operational-mode'.
> 
> Fix the example to use the correct property name.
> 
> Fixes: a9b121327c93 ("dt-bindings: soc: fsl: cpm_qe: Add QMC controller")
> Signed-off-by: Herve Codina 

Acked-by: Conor Dooley 

Thanks,
Conor.

> ---
>  .../bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml   | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git 
> a/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml 
> b/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml
> index ec888f48cac8..450a0354cb1d 100644
> --- a/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml
> +++ b/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml
> @@ -137,7 +137,7 @@ examples:
>  channel@16 {
>  /* Ch16 : First 4 even TS from all routed from TSA */
>  reg = <16>;
> -fsl,mode = "transparent";
> +fsl,operational-mode = "transparent";
>  fsl,reverse-data;
>  fsl,tx-ts-mask = <0x 0x00aa>;
>  fsl,rx-ts-mask = <0x 0x00aa>;
> @@ -146,7 +146,7 @@ examples:
>  channel@17 {
>  /* Ch17 : First 4 odd TS from all routed from TSA */
>  reg = <17>;
> -fsl,mode = "transparent";
> +fsl,operational-mode = "transparent";
>  fsl,reverse-data;
>  fsl,tx-ts-mask = <0x 0x0055>;
>  fsl,rx-ts-mask = <0x 0x0055>;
> @@ -155,7 +155,7 @@ examples:
>  channel@19 {
>  /* Ch19 : 8 TS (TS 8..15) from all routed from TSA */
>  reg = <19>;
> -fsl,mode = "hdlc";
> +fsl,operational-mode = "hdlc";
>  fsl,tx-ts-mask = <0x 0xff00>;
>  fsl,rx-ts-mask = <0x 0xff00>;
>  };
> -- 
> 2.41.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v2 05/28] dt-bindings: net: Add support for QMC HDLC

[Conor Dooley]

On Thu, Jul 27, 2023 at 11:09:48AM +0200, Herve Codina wrote:
> On Thu, 27 Jul 2023 09:19:59 +0100
> Conor Dooley  wrote:
> > On Wed, Jul 26, 2023 at 05:02:01PM +0200, Herve Codina wrote:

> If needed, I can change to:
>   title: QMC (QUICC Multichannel Controller) HDLC
> Let me known if it is better to you.

If it were me writing the binding, I'd probably use something like
"Freescale/NXP QUICC Multichannel Controller (QMC) HDLC", but it is not
a big deal, I just had a "wtf is this" moment :)



> > > +  fsl,qmc-chan:
> > 
> > Perhaps I am just showing my lack of knowledge in this area, but what is
> > fsl specific about wanting a reference to the channel of a "QMC"?
> > Is this something that hardware from other manufacturers would not also
> > want to do?
> 
> The QMC and the QMC channel are something specific to the SoC. This IP is only
> available on some Freescale/NXP SoCs.
> 
> When I upstreamed the 'fsl,qmc-audio.yaml', I first used a generic name for 
> this
> property and Kristoff asked to change to a vendor prefixed name.
>   
> https://lore.kernel.org/linux-kernel/1dfade07-f8c4-2e16-00dc-c7d183708...@linaro.org/
> 
> Based on this, as the property 'fsl,qmc-chan' has the exact same meaning in
> fsl,qmc-audio.yaml and fsl,qmc-hdlc.yaml, I use the same name.

Okay, thanks for explaining!


signature.asc
Description: PGP signature

Re: [PATCH v2 05/28] dt-bindings: net: Add support for QMC HDLC

[Conor Dooley]

On Wed, Jul 26, 2023 at 05:02:01PM +0200, Herve Codina wrote:
> The QMC (QUICC mutichannel controller) is a controller present in some
> PowerQUICC SoC such as MPC885.
> The QMC HDLC uses the QMC controller to transfer HDLC data.
> 
> Signed-off-by: Herve Codina 
> ---
>  .../devicetree/bindings/net/fsl,qmc-hdlc.yaml | 41 +++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/fsl,qmc-hdlc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/fsl,qmc-hdlc.yaml 
> b/Documentation/devicetree/bindings/net/fsl,qmc-hdlc.yaml
> new file mode 100644
> index ..8bb6f34602d9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/fsl,qmc-hdlc.yaml
> @@ -0,0 +1,41 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/fsl,qmc-hdlc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: QMC HDLC

"QMC HDLC" seems excessively terse.

> +
> +maintainers:
> +  - Herve Codina 
> +
> +description: |
> +  The QMC HDLC uses a QMC (QUICC Multichannel Controller) channel to transfer
> +  HDLC data.
> +
> +properties:
> +  compatible:
> +const: fsl,qmc-hdlc
> +
> +  fsl,qmc-chan:

Perhaps I am just showing my lack of knowledge in this area, but what is
fsl specific about wanting a reference to the channel of a "QMC"?
Is this something that hardware from other manufacturers would not also
want to do?

> +$ref: /schemas/types.yaml#/definitions/phandle-array
> +items:
> +  - items:
> +  - description: phandle to QMC node
> +  - description: Channel number
> +description:
> +  Should be a phandle/number pair. The phandle to QMC node and the QMC
> +  channel to use.
> +
> +required:
> +  - compatible
> +  - fsl,qmc-chan
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +hdlc {
> +compatible = "fsl,qmc-hdlc";
> +fsl,qmc-chan = <&qmc 16>;
> +};
> -- 
> 2.41.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v2 27/28] dt-bindings: net: fsl,qmc-hdlc: Add framer support

[Conor Dooley]

On Wed, Jul 26, 2023 at 05:02:23PM +0200, Herve Codina wrote:
> A framer can be connected to the QMC HDLC.
> If present, this framer is the interface between the TDM used by the QMC
> HDLC and the E1/T1 line.
> The QMC HDLC can use this framer to get information about the line and
> configure the line.
> 
> Add an optional framer property to reference the framer itself.
> 
> Signed-off-by: Herve Codina 

Why not fully describe the hardware in one patch in this series, rather
than split this over two different ones?

> ---
>  Documentation/devicetree/bindings/net/fsl,qmc-hdlc.yaml | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/fsl,qmc-hdlc.yaml 
> b/Documentation/devicetree/bindings/net/fsl,qmc-hdlc.yaml
> index 8bb6f34602d9..bf29863ab419 100644
> --- a/Documentation/devicetree/bindings/net/fsl,qmc-hdlc.yaml
> +++ b/Documentation/devicetree/bindings/net/fsl,qmc-hdlc.yaml
> @@ -27,6 +27,11 @@ properties:
>Should be a phandle/number pair. The phandle to QMC node and the QMC
>channel to use.
>  
> +  framer:
> +$ref: /schemas/types.yaml#/definitions/phandle
> +description:
> +  phandle to the framer node
> +
>  required:
>- compatible
>- fsl,qmc-chan
> -- 
> 2.41.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v3 11/13] riscv/kexec: refactor for kernel/Kconfig.kexec

[Conor Dooley]

Hey Eric,

On Mon, Jun 26, 2023 at 12:13:30PM -0400, Eric DeVolder wrote:
> The kexec and crash kernel options are provided in the common
> kernel/Kconfig.kexec. Utilize the common options and provide
> the ARCH_SUPPORTS_ and ARCH_SELECTS_ entries to recreate the

> equivalent set of KEXEC and CRASH options.

I find this diff a little hard to follow (since the other half off the
change is in another patch), so it may be me missing something, but are
you sure?

> 
> Signed-off-by: Eric DeVolder 
> ---
>  arch/riscv/Kconfig | 48 ++
>  1 file changed, 14 insertions(+), 34 deletions(-)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 5966ad97c30c..c484abd9bbfd 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -585,48 +585,28 @@ config RISCV_BOOT_SPINWAIT
>  
> If unsure what to do here, say N.
>  
> -config KEXEC
> - bool "Kexec system call"
> - depends on MMU
> +config ARCH_SUPPORTS_KEXEC
> + def_bool MMU
> +
> +config ARCH_SELECTS_KEXEC
> + def_bool y
> + depends on KEXEC
>   select HOTPLUG_CPU if SMP
> - select KEXEC_CORE
> - help
> -   kexec is a system call that implements the ability to shutdown your
> -   current kernel, and to start another kernel. It is like a reboot
> -   but it is independent of the system firmware. And like a reboot
> -   you can start any kernel with it, not just Linux.
>  
> -   The name comes from the similarity to the exec system call.
> +config ARCH_SUPPORTS_KEXEC_FILE
> + def_bool 64BIT && MMU && CRYPTO=y && CRYPTO_SHA256=y

This looks like a change to me. Previously, only KEXEC_PURGATORY
required these crypto options to be like so, but now KEXEC_FILE needs
them too.

What am I missing?

Cheers,
Conor.

>  
> -config KEXEC_FILE
> - bool "kexec file based systmem call"
> - depends on 64BIT && MMU
> - select HAVE_IMA_KEXEC if IMA
> - select KEXEC_CORE
> +config ARCH_SELECTS_KEXEC_FILE
> + def_bool y
> + depends on KEXEC_FILE
>   select KEXEC_ELF
> - help
> -   This is new version of kexec system call. This system call is
> -   file based and takes file descriptors as system call argument
> -   for kernel and initramfs as opposed to list of segments as
> -   accepted by previous system call.
> -
> -   If you don't know what to do here, say Y.
> + select HAVE_IMA_KEXEC if IMA
>  
>  config ARCH_HAS_KEXEC_PURGATORY
>   def_bool KEXEC_FILE
> - depends on CRYPTO=y
> - depends on CRYPTO_SHA256=y
>  
> -config CRASH_DUMP
> - bool "Build kdump crash kernel"
> - help
> -   Generate crash dump after being started by kexec. This should
> -   be normally only set in special crash dump kernels which are
> -   loaded in the main kernel with kexec-tools into a specially
> -   reserved region and then later executed after a crash by
> -   kdump/kexec.
> -
> -   For more details see Documentation/admin-guide/kdump/kdump.rst
> +config ARCH_SUPPORTS_CRASH_DUMP
> + def_bool y
>  
>  config COMPAT
>   bool "Kernel support for 32-bit U-mode"
> -- 
> 2.31.1
>


signature.asc
Description: PGP signature

Re: [PATCH v6 4/4] risc/purgatory: Add linker script

[Conor Dooley]

On Mon, May 01, 2023 at 09:54:43PM +0200, Ricardo Ribalda wrote:
> On Mon, 1 May 2023 at 19:41, Conor Dooley  wrote:
> > On Mon, May 01, 2023 at 02:38:22PM +0200, Ricardo Ribalda wrote:
> > > If PGO is enabled, the purgatory ends up with multiple .text sections.
> > > This is not supported by kexec and crashes the system.
> > >
> > > Cc: sta...@vger.kernel.org
> > > Fixes: 930457057abe ("kernel/kexec_file.c: split up 
> > > __kexec_load_puragory")
> > > Signed-off-by: Ricardo Ribalda 
> > > ---
> > >  arch/riscv/purgatory/Makefile | 5 +
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/arch/riscv/purgatory/Makefile b/arch/riscv/purgatory/Makefile
> > > index 5730797a6b40..cf3a44121a90 100644
> > > --- a/arch/riscv/purgatory/Makefile
> > > +++ b/arch/riscv/purgatory/Makefile
> > > @@ -35,6 +35,11 @@ CFLAGS_sha256.o := -D__DISABLE_EXPORTS
> > >  CFLAGS_string.o := -D__DISABLE_EXPORTS
> > >  CFLAGS_ctype.o := -D__DISABLE_EXPORTS
> > >
> > > +# When profile optimization is enabled, llvm emits two different 
> > > overlapping
> > > +# text sections, which is not supported by kexec. Remove profile 
> > > optimization
> > > +# flags.
> > > +KBUILD_CFLAGS := $(filter-out -fprofile-sample-use=% 
> > > -fprofile-use=%,$(KBUILD_CFLAGS))
> >
> > With the caveat of not being au fait with the workings of either PGO or
> > of purgatory, how come you modify KBUILD_CFLAGS here rather than the
> > purgatory specific PURGATORY_CFLAGS that are used later in the file?
> 
> Definitely, not a Makefile expert here, but when I tried this:
> 
> @@ -35,6 +40,7 @@ PURGATORY_CFLAGS_REMOVE := -mcmodel=kernel
>  PURGATORY_CFLAGS := -mcmodel=large -ffreestanding
> -fno-zero-initialized-in-bss -g0
>  PURGATORY_CFLAGS += $(DISABLE_STACKLEAK_PLUGIN) -DDISABLE_BRANCH_PROFILING
>  PURGATORY_CFLAGS += -fno-stack-protector
> +PURGATORY_CFLAGS := $(filter-out -fprofile-sample-use=%
> -fprofile-use=%,$(KBUILD_CFLAGS))
> 
> It did not work.

Unfortunately I am oh-so-far from an expert on this kind of thing, but I
had thought that PURGATORY_CFLAGS_REMOVE was intended for this sort of
purpose.

> Fixes: bde971a83bbf ("KVM: arm64: nvhe: Fix build with profile optimization")
> 
> does this approach, so this is what I tried and worked.

That doesn't have a specific CFLAGS though afaict.
Perhaps Nick etc have a more informed opinion here than I do, sorry.

Thanks,
Conor.

> > > +
> > >  # When linking purgatory.ro with -r unresolved symbols are not checked,
> > >  # also link a purgatory.chk binary without -r to check for unresolved 
> > > symbols.
> > >  PURGATORY_LDFLAGS := -e purgatory_start -z nodefaultlib
> > >
> > > --
> > > 2.40.1.495.gc816e09b53d-goog
> > >
> > >
> > > ___
> > > linux-riscv mailing list
> > > linux-ri...@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> 
> 
> 
> -- 
> Ricardo Ribalda


signature.asc
Description: PGP signature

Re: [PATCH v6 4/4] risc/purgatory: Add linker script

[Conor Dooley]

Hey Ricardo,

On Mon, May 01, 2023 at 02:38:22PM +0200, Ricardo Ribalda wrote:
> If PGO is enabled, the purgatory ends up with multiple .text sections.
> This is not supported by kexec and crashes the system.
> 
> Cc: sta...@vger.kernel.org
> Fixes: 930457057abe ("kernel/kexec_file.c: split up __kexec_load_puragory")
> Signed-off-by: Ricardo Ribalda 
> ---
>  arch/riscv/purgatory/Makefile | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/riscv/purgatory/Makefile b/arch/riscv/purgatory/Makefile
> index 5730797a6b40..cf3a44121a90 100644
> --- a/arch/riscv/purgatory/Makefile
> +++ b/arch/riscv/purgatory/Makefile
> @@ -35,6 +35,11 @@ CFLAGS_sha256.o := -D__DISABLE_EXPORTS
>  CFLAGS_string.o := -D__DISABLE_EXPORTS
>  CFLAGS_ctype.o := -D__DISABLE_EXPORTS
>  
> +# When profile optimization is enabled, llvm emits two different overlapping
> +# text sections, which is not supported by kexec. Remove profile optimization
> +# flags.
> +KBUILD_CFLAGS := $(filter-out -fprofile-sample-use=% 
> -fprofile-use=%,$(KBUILD_CFLAGS))

With the caveat of not being au fait with the workings of either PGO or
of purgatory, how come you modify KBUILD_CFLAGS here rather than the
purgatory specific PURGATORY_CFLAGS that are used later in the file?

Cheers,
Conor.

> +
>  # When linking purgatory.ro with -r unresolved symbols are not checked,
>  # also link a purgatory.chk binary without -r to check for unresolved 
> symbols.
>  PURGATORY_LDFLAGS := -e purgatory_start -z nodefaultlib
> 
> -- 
> 2.40.1.495.gc816e09b53d-goog
> 
> 
> ___
> linux-riscv mailing list
> linux-ri...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv


signature.asc
Description: PGP signature

Re: [PATCH v6 4/4] risc/purgatory: Add linker script

[Conor Dooley]

On Mon, May 01, 2023 at 07:18:12PM +0200, Ricardo Ribalda wrote:
> On Mon, 1 May 2023 at 18:19, Nick Desaulniers  wrote:
> >
> > On Mon, May 1, 2023 at 5:39 AM Ricardo Ribalda  wrote:
> > >
> > > If PGO is enabled, the purgatory ends up with multiple .text sections.
> > > This is not supported by kexec and crashes the system.
> > >
> > > Cc: sta...@vger.kernel.org
> > > Fixes: 930457057abe ("kernel/kexec_file.c: split up 
> > > __kexec_load_puragory")
> > > Signed-off-by: Ricardo Ribalda 
> >
> > Hi Ricardo,
> > Thanks for the series.  Does this patch 4/4 need a new online commit
> > description? It's not adding a linker script (maybe an earlier version
> > was).

> Thanks for catching this. It should have said
> 
> risc/purgatory: Remove profile optimization flags
 ^^
Perhaps with the omitted v added too?

Also while playing the $subject nitpicking game, is it not called
"profile**-guided** optimisation" (and ditto in the comments)?

Cheers,
Conor.

> Will fix it on my local branch in case there is a next version of the
> series. Otherwise, please the maintainer fix the subject.

> > > ---
> > >  arch/riscv/purgatory/Makefile | 5 +
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/arch/riscv/purgatory/Makefile b/arch/riscv/purgatory/Makefile
> > > index 5730797a6b40..cf3a44121a90 100644
> > > --- a/arch/riscv/purgatory/Makefile
> > > +++ b/arch/riscv/purgatory/Makefile
> > > @@ -35,6 +35,11 @@ CFLAGS_sha256.o := -D__DISABLE_EXPORTS
> > >  CFLAGS_string.o := -D__DISABLE_EXPORTS
> > >  CFLAGS_ctype.o := -D__DISABLE_EXPORTS
> > >
> > > +# When profile optimization is enabled, llvm emits two different 
> > > overlapping
> > > +# text sections, which is not supported by kexec. Remove profile 
> > > optimization
> > > +# flags.
> > > +KBUILD_CFLAGS := $(filter-out -fprofile-sample-use=% 
> > > -fprofile-use=%,$(KBUILD_CFLAGS))
> > > +
> > >  # When linking purgatory.ro with -r unresolved symbols are not checked,
> > >  # also link a purgatory.chk binary without -r to check for unresolved 
> > > symbols.
> > >  PURGATORY_LDFLAGS := -e purgatory_start -z nodefaultlib
> > >
> > > --
> > > 2.40.1.495.gc816e09b53d-goog
> > >
> >
> >
> > --
> > Thanks,
> > ~Nick Desaulniers
> 
> 
> 
> -- 
> Ricardo Ribalda
> 
> ___
> linux-riscv mailing list
> linux-ri...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv


signature.asc
Description: PGP signature

Re: [PATCH v9 0/6] Introduce 64b relocatable kernel

[Conor Dooley]

On Fri, Apr 21, 2023 at 07:10:14PM +, Konstantin Ryabitsev wrote:
> April 21, 2023 2:59 PM, "Palmer Dabbelt"  wrote:
> >> riscv: Use PUD/P4D/PGD pages for the linear mapping
> >> (https://patchwork.kernel.org/project/linux-riscv/list/?series=733603)
> >> base-commit-tag: v6.3-rc1
> > 
> > The QEMU CI has some way to say "this depends on an un-merged patch set 
> > sent as $MESSAGE_ID", not
> > sure if that's a b4-ism but it's a bit less confusing.
> 
> I think it's patchwork-ism, actually. B4 will eventually learn to be
> able to include dependent series info and automatically retrieve/apply
> them in the proper order on "shazam", but it can't do that yet.

A patchwork-ism or a patchew-ism? Drew Jones was my source for this, but
he had said the thing to do in QEMU-land was put a:
Based-on: $message-id
in your cover letter for each thing that you depend on. I'm not entirely
sure if that meant each series or each patch. I think patchew picks that
up and dumps in it on a patchew github account that the CI might pick up
on. From the QEMU docs:

It is also okay to base patches on top of other on-going work that is
not yet part of the git master branch. To aid continuous integration
tools, such as `patchew `__, you should `add a
tag `__
line ``Based-on: $MESSAGE_ID`` to your cover letter to make the series
dependency obvious.
<\quote>

FWIW, my vote is for something with a message-id, rather than those
patchwork series links that you can't dump into b4!


signature.asc
Description: PGP signature

[PATCH v1] dt-bindings: move cache controller bindings to a cache directory

[Conor Dooley]

From: Conor Dooley 

There's a bunch of bindings for (mostly l2) cache controllers
scattered to the four winds, move them to a common directory.
I renamed the freescale l2cache.txt file, as while that might make sense
when the parent dir is fsl, it's confusing after the move.
The two Marvell bindings have had a "marvell," prefix added to match
their compatibles.

Signed-off-by: Conor Dooley 
---
 .../{memory-controllers => cache}/baikal,bt1-l2-ctl.yaml| 2 +-
 .../{powerpc/fsl/l2cache.txt => cache/freescale-l2cache.txt}| 0
 Documentation/devicetree/bindings/{arm => cache}/l2c2x0.yaml| 2 +-
 .../{arm/mrvl/feroceon.txt => cache/marvell,feroceon-cache.txt} | 0
 .../{arm/mrvl/tauros2.txt => cache/marvell,tauros2-cache.txt}   | 0
 .../devicetree/bindings/{arm/msm => cache}/qcom,llcc.yaml   | 2 +-
 .../devicetree/bindings/{riscv => cache}/sifive,ccache0.yaml| 2 +-
 .../socionext => cache}/socionext,uniphier-system-cache.yaml| 2 +-
 MAINTAINERS | 2 ++
 9 files changed, 7 insertions(+), 5 deletions(-)
 rename Documentation/devicetree/bindings/{memory-controllers => 
cache}/baikal,bt1-l2-ctl.yaml (95%)
 rename Documentation/devicetree/bindings/{powerpc/fsl/l2cache.txt => 
cache/freescale-l2cache.txt} (100%)
 rename Documentation/devicetree/bindings/{arm => cache}/l2c2x0.yaml (99%)
 rename Documentation/devicetree/bindings/{arm/mrvl/feroceon.txt => 
cache/marvell,feroceon-cache.txt} (100%)
 rename Documentation/devicetree/bindings/{arm/mrvl/tauros2.txt => 
cache/marvell,tauros2-cache.txt} (100%)
 rename Documentation/devicetree/bindings/{arm/msm => cache}/qcom,llcc.yaml 
(96%)
 rename Documentation/devicetree/bindings/{riscv => cache}/sifive,ccache0.yaml 
(98%)
 rename Documentation/devicetree/bindings/{arm/socionext => 
cache}/socionext,uniphier-system-cache.yaml (96%)

diff --git 
a/Documentation/devicetree/bindings/memory-controllers/baikal,bt1-l2-ctl.yaml 
b/Documentation/devicetree/bindings/cache/baikal,bt1-l2-ctl.yaml
similarity index 95%
rename from 
Documentation/devicetree/bindings/memory-controllers/baikal,bt1-l2-ctl.yaml
rename to Documentation/devicetree/bindings/cache/baikal,bt1-l2-ctl.yaml
index 1fca282f64a2..ec4f367bc0b4 100644
--- 
a/Documentation/devicetree/bindings/memory-controllers/baikal,bt1-l2-ctl.yaml
+++ b/Documentation/devicetree/bindings/cache/baikal,bt1-l2-ctl.yaml
@@ -2,7 +2,7 @@
 # Copyright (C) 2020 BAIKAL ELECTRONICS, JSC
 %YAML 1.2
 ---
-$id: http://devicetree.org/schemas/memory-controllers/baikal,bt1-l2-ctl.yaml#
+$id: http://devicetree.org/schemas/cache/baikal,bt1-l2-ctl.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
 title: Baikal-T1 L2-cache Control Block
diff --git a/Documentation/devicetree/bindings/powerpc/fsl/l2cache.txt 
b/Documentation/devicetree/bindings/cache/freescale-l2cache.txt
similarity index 100%
rename from Documentation/devicetree/bindings/powerpc/fsl/l2cache.txt
rename to Documentation/devicetree/bindings/cache/freescale-l2cache.txt
diff --git a/Documentation/devicetree/bindings/arm/l2c2x0.yaml 
b/Documentation/devicetree/bindings/cache/l2c2x0.yaml
similarity index 99%
rename from Documentation/devicetree/bindings/arm/l2c2x0.yaml
rename to Documentation/devicetree/bindings/cache/l2c2x0.yaml
index 6b8f4d4fa580..d7840a5c4037 100644
--- a/Documentation/devicetree/bindings/arm/l2c2x0.yaml
+++ b/Documentation/devicetree/bindings/cache/l2c2x0.yaml
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 %YAML 1.2
 ---
-$id: http://devicetree.org/schemas/arm/l2c2x0.yaml#
+$id: http://devicetree.org/schemas/cache/l2c2x0.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
 title: ARM L2 Cache Controller
diff --git a/Documentation/devicetree/bindings/arm/mrvl/feroceon.txt 
b/Documentation/devicetree/bindings/cache/marvell,feroceon-cache.txt
similarity index 100%
rename from Documentation/devicetree/bindings/arm/mrvl/feroceon.txt
rename to Documentation/devicetree/bindings/cache/marvell,feroceon-cache.txt
diff --git a/Documentation/devicetree/bindings/arm/mrvl/tauros2.txt 
b/Documentation/devicetree/bindings/cache/marvell,tauros2-cache.txt
similarity index 100%
rename from Documentation/devicetree/bindings/arm/mrvl/tauros2.txt
rename to Documentation/devicetree/bindings/cache/marvell,tauros2-cache.txt
diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml 
b/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
similarity index 96%
rename from Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml
rename to Documentation/devicetree/bindings/cache/qcom,llcc.yaml
index 38efcad56dbd..14eb5175dac4 100644
--- a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml
+++ b/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
 %YAML 1.2
 ---
-$id: http://devicetree.org/schemas/arm/msm/qcom,llcc.yaml#
+$id

Re: [PATCH 08/21] riscv: dma-mapping: only invalidate after DMA, not flush

[Conor Dooley]

On Mon, Mar 27, 2023 at 02:13:04PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann 
> 
> No other architecture intentionally writes back dirty cache lines into
> a buffer that a device has just finished writing into. If the cache is
> clean, this has no effect at all, but

> if a cacheline in the buffer has
> actually been written by the CPU,  there is a drive bug that is likely
> made worse by overwriting that buffer.

So does this need a
Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom 
extension")
then, even if the cacheline really should not have been touched by the
CPU?
Also, minor typo, s/drive/driver/.

In the thread we had that sparked this, I went digging for the source of
the flushes, and it came from a review comment:
https://lore.kernel.org/linux-riscv/342e3c12-ebb0-badf-7d4c-c444a2b84...@sholland.org/
But *surely* if no other arch needs to do that, then we are safe to also
not do it... Your logic seems right by me at least, especially given the
lack of flushes elsewhere.
Reviewed-by: Conor Dooley 

Cheers,
Conor.

> Signed-off-by: Arnd Bergmann 
> ---
>  arch/riscv/mm/dma-noncoherent.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
> index d919efab6eba..640f4c496d26 100644
> --- a/arch/riscv/mm/dma-noncoherent.c
> +++ b/arch/riscv/mm/dma-noncoherent.c
> @@ -42,7 +42,7 @@ void arch_sync_dma_for_cpu(phys_addr_t paddr, size_t size,
>   break;
>   case DMA_FROM_DEVICE:
>   case DMA_BIDIRECTIONAL:
> - ALT_CMO_OP(flush, vaddr, size, riscv_cbom_block_size);
> + ALT_CMO_OP(inval, vaddr, size, riscv_cbom_block_size);
>   break;
>   default:
>   break;
> -- 
> 2.39.2
> 


signature.asc
Description: PGP signature

Re: [PATCH 09/21] riscv: dma-mapping: skip invalidation before bidirectional DMA

[Conor Dooley]

On Mon, Mar 27, 2023 at 02:13:05PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann 
> 
> For a DMA_BIDIRECTIONAL transfer, the caches have to be cleaned
> first to let the device see data written by the CPU, and invalidated
> after the transfer to let the CPU see data written by the device.
> 
> riscv also invalidates the caches before the transfer, which does
> not appear to serve any purpose.

Rationale makes sense to me..
Reviewed-by: Conor Dooley 

Thanks for working on all of this Arnd!


signature.asc
Description: PGP signature

Re: [PATCH v2 4/5] riscv: Select ARCH_DMA_DEFAULT_COHERENT

[Conor Dooley]

On Thu, Feb 23, 2023 at 11:36:43AM +, Jiaxun Yang wrote:
> For riscv our assumption is unless a device states it is non-coherent,
> we take it to be DMA coherent.
> 
> Select ARCH_DMA_DEFAULT_COHERENT to ensure dma_default_coherent
> is always initialized to true.
> 
> Signed-off-by: Jiaxun Yang 
> ---
>  arch/riscv/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 1d46a268ce16..b71ce992c0c0 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -233,6 +233,7 @@ config LOCKDEP_SUPPORT
>  
>  config RISCV_DMA_NONCOHERENT
>   bool
> + select ARCH_DMA_DEFAULT_COHERENT

Since we are always coherent by default, I feel like you should put this
in the main "config RISCV" section, where OF_DMA_DEFAULT_COHERENT
currently is, no?

Wouldn't bother respinning for that unless the dma folk have comments
for you.

>   select ARCH_HAS_DMA_PREP_COHERENT
>   select ARCH_HAS_SETUP_DMA_OPS
>   select ARCH_HAS_SYNC_DMA_FOR_CPU
> -- 
> 2.37.1 (Apple Git-137.1)
> 


signature.asc
Description: PGP signature

Re: [PATCH 2/3] riscv: Set dma_default_coherent to true

[Conor Dooley]

On Wed, Feb 22, 2023 at 04:20:16PM +, Jiaxun Yang wrote:
> > 2023年2月22日 16:02，Conor Dooley  写道：
> > On Wed, Feb 22, 2023 at 03:55:19PM +, Jiaxun Yang wrote:
> >>> 2023年2月22日 14:50，Conor Dooley  写道：
> >>> On Wed, Feb 22, 2023 at 01:37:11PM +, Jiaxun Yang wrote:
> >>>> For riscv our assumption is unless a device states it is non-coherent,
> >>>> we take it to be DMA coherent.
> >>>> 
> >>>> For devicetree probed devices that have been true since very begining
> >>>> with OF_DMA_DEFAULT_COHERENT selected.
> >>>> 
> >>>> Signed-off-by: Jiaxun Yang 
> >>>> ---
> >>>> arch/riscv/kernel/setup.c | 3 +++
> >>>> 1 file changed, 3 insertions(+)
> >>>> 
> >>>> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> >>>> index 376d2827e736..34b371180976 100644
> >>>> --- a/arch/riscv/kernel/setup.c
> >>>> +++ b/arch/riscv/kernel/setup.c
> >>>> @@ -300,6 +300,9 @@ void __init setup_arch(char **cmdline_p)
> >>>> riscv_init_cbom_blocksize();
> >>>> riscv_fill_hwcap();
> >>>> apply_boot_alternatives();
> >>>> +#ifdef CONFIG_RISCV_DMA_NONCOHERENT
> >>>> + dma_default_coherent = true;
> >>>> +#endif
> >>> 
> >>> Do we really need to add ifdeffery for this here?
> >>> It's always coherent by default, so why do we need to say set it in
> >>> setup_arch() when we know that, regardless of options, it is true?
> >> 
> >> Because this symbol is only a variable when:
> >> 
> >> defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
> >> defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
> >> defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
> >> 
> >> Which is only true if  CONFIG_RISCV_DMA_NONCOHERENT is selected.
> >> 
> >> Otherwise this symbol is defined to true and we can’t make a assignment to 
> >> it.

> > Maybe I am just slow today, but I don't get why you need to add
> > ifdeffery to setup_arch() to do something that is always true.
> > Why can't you just set this in riscv/mm/dma-noncoherent.c? What am I
> > missing?
> 
> Hmm that sounds like a good idea but I was unable to find a place for this.
> 
> riscv/mm/dma-noncoherent.c are just a bunch of callbacks, there is no 
> initialisation
> function that will always run on every boot. riscv_noncoherent_supported is 
> only
> called conditionally.

Right, that's fair. riscv_noncoherent_supported() is for when we know we
can support non-coherent DMA, not to detect whether we can, hence the
conditional nature.
Apologies for sending you on a wild goose chase there.

> Perhaps I can add a initcall in dma-noncoherent.c but it seems to be a little 
> bit
> overkilling.

Yah, probably would be.

> Actually I’d prefer to have a config option for the default value but it 
> seems like
> it’s not in Christoph’s flavour.

We already have a config option that sets things up nicely for RISC-V
that you're getting rid of! ;)


signature.asc
Description: PGP signature

Re: [PATCH 2/3] riscv: Set dma_default_coherent to true

[Conor Dooley]

On Wed, Feb 22, 2023 at 03:55:19PM +, Jiaxun Yang wrote:
> 
> 
> > 2023年2月22日 14:50，Conor Dooley  写道：
> > 
> > On Wed, Feb 22, 2023 at 01:37:11PM +, Jiaxun Yang wrote:
> >> For riscv our assumption is unless a device states it is non-coherent,
> >> we take it to be DMA coherent.
> >> 
> >> For devicetree probed devices that have been true since very begining
> >> with OF_DMA_DEFAULT_COHERENT selected.
> >> 
> >> Signed-off-by: Jiaxun Yang 
> >> ---
> >> arch/riscv/kernel/setup.c | 3 +++
> >> 1 file changed, 3 insertions(+)
> >> 
> >> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> >> index 376d2827e736..34b371180976 100644
> >> --- a/arch/riscv/kernel/setup.c
> >> +++ b/arch/riscv/kernel/setup.c
> >> @@ -300,6 +300,9 @@ void __init setup_arch(char **cmdline_p)
> >> riscv_init_cbom_blocksize();
> >> riscv_fill_hwcap();
> >> apply_boot_alternatives();
> >> +#ifdef CONFIG_RISCV_DMA_NONCOHERENT
> >> + dma_default_coherent = true;
> >> +#endif
> > 
> > Do we really need to add ifdeffery for this here?
> > It's always coherent by default, so why do we need to say set it in
> > setup_arch() when we know that, regardless of options, it is true?
> 
> Because this symbol is only a variable when:
> 
> defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
> defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
> defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
> 
> Which is only true if  CONFIG_RISCV_DMA_NONCOHERENT is selected.
> 
> Otherwise this symbol is defined to true and we can’t make a assignment to it.

Maybe I am just slow today, but I don't get why you need to add
ifdeffery to setup_arch() to do something that is always true.
Why can't you just set this in riscv/mm/dma-noncoherent.c? What am I
missing?

Cheers,
Conor.

> >> if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM) &&
> >> riscv_isa_extension_available(NULL, ZICBOM))
> >> riscv_noncoherent_supported();
> >> -- 
> >> 2.37.1 (Apple Git-137.1)
> 
> 


signature.asc
Description: PGP signature

Re: [PATCH 2/3] riscv: Set dma_default_coherent to true

[Conor Dooley]

On Wed, Feb 22, 2023 at 01:37:11PM +, Jiaxun Yang wrote:
> For riscv our assumption is unless a device states it is non-coherent,
> we take it to be DMA coherent.
> 
> For devicetree probed devices that have been true since very begining
> with OF_DMA_DEFAULT_COHERENT selected.
> 
> Signed-off-by: Jiaxun Yang 
> ---
>  arch/riscv/kernel/setup.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index 376d2827e736..34b371180976 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -300,6 +300,9 @@ void __init setup_arch(char **cmdline_p)
>   riscv_init_cbom_blocksize();
>   riscv_fill_hwcap();
>   apply_boot_alternatives();
> +#ifdef CONFIG_RISCV_DMA_NONCOHERENT
> + dma_default_coherent = true;
> +#endif

Do we really need to add ifdeffery for this here?
It's always coherent by default, so why do we need to say set it in
setup_arch() when we know that, regardless of options, it is true?

Cheers,
Conor.

>   if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM) &&
>   riscv_isa_extension_available(NULL, ZICBOM))
>   riscv_noncoherent_supported();
> -- 
> 2.37.1 (Apple Git-137.1)
> 


signature.asc
Description: PGP signature

Re: [PATCH 6/7] riscv: Select ARCH_DMA_DEFAULT_COHERENT

[Conor Dooley]

On Tue, Feb 21, 2023 at 12:46:12PM +, Jiaxun Yang wrote:
> For RISCV we always assume devices are DMA coherent.

"Always assume", I'm not keen on that wording as it is unclear as to
whether you are suggesting that a) we always take devices to be DMA
coherent, or b) unless a device states it is non-coherent, we take it to
be DMA coherent.
I think you mean b, but being exact would be appreciated, thanks.

> Select ARCH_DMA_DEFAULT_COHERENT to ensure dev->dma_conherent
> is always initialized to true.
> 
> Signed-off-by: Jiaxun Yang 

Why was this not sent to the riscv list?
When (or if) you send v2, can you please make sure that you do cc it?

Thanks,
Conor.


signature.asc
Description: PGP signature

Re: [PATCH v2 4/4] mm, arch: add generic implementation of pfn_valid() for FLATMEM

[Conor Dooley]

Hi Mike,

On Tue, Jan 31, 2023 at 08:41:49PM +0200, Mike Rapoport wrote:
> On Tue, Jan 31, 2023 at 05:47:24PM +0000, Conor Dooley wrote:
> > On Sun, Jan 29, 2023 at 02:42:35PM +0200, Mike Rapoport wrote:
> > > From: "Mike Rapoport (IBM)" 
> > > 
> > > Every architecture that supports FLATMEM memory model defines its own
> > > version of pfn_valid() that essentially compares a pfn to max_mapnr.
> > > 
> > > Use mips/powerpc version implemented as static inline as a generic
> > > implementation of pfn_valid() and drop its per-architecture definitions.
> > > 
> > > Signed-off-by: Mike Rapoport (IBM) 
> > > Acked-by: Arnd Bergmann 
> > > Acked-by: Guo Ren  # csky
> > > Acked-by: Huacai Chen # LoongArch
> > > Acked-by: Stafford Horne# OpenRISC
> > 
> > Hmm, so this landed in linux-next today and I bisected a boot failure in
> > my CI to it. However, I am not really sure if it is a real issue worth
> > worrying about as the platform it triggered on is supposed to be using
> > SPARSEMEM, but isn't.
> > I had thought that my CI was using a config with SPARSEMEM since that
> > became required for riscv defconfig builds to boot in v6.1-rc1, but I
> > must have just forgotten to add it to my $platform_defconfig builds too.
> > However, those $platform_defconfig builds continued booting without
> > SPARSEMEM enabled until today.
> 
> The issue seems to be that the generic pfn_valid() does not take into
> account pfn_offset when it compares it with max_mapnr.
> Can you please test with the patch below?
> 
> diff --git a/include/asm-generic/memory_model.h 
> b/include/asm-generic/memory_model.h
> index 13d2a844d928..6796abe1900e 100644
> --- a/include/asm-generic/memory_model.h
> +++ b/include/asm-generic/memory_model.h
> @@ -26,7 +26,7 @@ static inline int pfn_valid(unsigned long pfn)
>   extern unsigned long max_mapnr;
>   unsigned long pfn_offset = ARCH_PFN_OFFSET;
>  
> - return pfn >= pfn_offset && pfn < max_mapnr;
> + return pfn >= pfn_offset && (pfn - pfn_offset) < max_mapnr;
>  }
>  #define pfn_valid pfn_valid
>  #endif

Gave that a go, board is booting properly again! Feel free to add a:
Tested-by: Conor Dooley 

Thanks for the prompt fix!



signature.asc
Description: PGP signature

Re: [PATCH v2 4/4] mm, arch: add generic implementation of pfn_valid() for FLATMEM

[Conor Dooley]

Hey Mike,

On Sun, Jan 29, 2023 at 02:42:35PM +0200, Mike Rapoport wrote:
> From: "Mike Rapoport (IBM)" 
> 
> Every architecture that supports FLATMEM memory model defines its own
> version of pfn_valid() that essentially compares a pfn to max_mapnr.
> 
> Use mips/powerpc version implemented as static inline as a generic
> implementation of pfn_valid() and drop its per-architecture definitions.
> 
> Signed-off-by: Mike Rapoport (IBM) 
> Acked-by: Arnd Bergmann 
> Acked-by: Guo Ren  # csky
> Acked-by: Huacai Chen # LoongArch
> Acked-by: Stafford Horne# OpenRISC

Hmm, so this landed in linux-next today and I bisected a boot failure in
my CI to it. However, I am not really sure if it is a real issue worth
worrying about as the platform it triggered on is supposed to be using
SPARSEMEM, but isn't.
I had thought that my CI was using a config with SPARSEMEM since that
became required for riscv defconfig builds to boot in v6.1-rc1, but I
must have just forgotten to add it to my $platform_defconfig builds too.
However, those $platform_defconfig builds continued booting without
SPARSEMEM enabled until today.
for $platform_defconfig builds I now see the following, which is in turn
followed by an opps. The full output is at the bottom of this mail.

WARNING: CPU: 0 PID: 0 at mm/vmalloc.c:479 
__vmap_pages_range_noflush+0x2d4/0x4c8
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 6.2.0-rc4-00538-g6e265f6be0c8 
#1
Hardware name: Microchip PolarFire-SoC Icicle Kit (DT)
epc : __vmap_pages_range_noflush+0x2d4/0x4c8
 ra : __vmap_pages_range_noflush+0x3e0/0x4c8
epc : 801461e4 ra : 801462f0 sp : 81203b90
 gp : 812e9fe0 tp : 8120fa80 t0 : ffe7bfe57000
 t1 : 8000 t2 : 01a7 s0 : 81203c50
 s1 : ffe7bfe56040 a0 :  a1 : ffe7c7df2840
 a2 : 812ebae8 a3 : 1000 a4 : 00080200
 a5 : 00fffe00 a6 : 01040056 a7 : 
 s2 : 01040024 s3 : 80de1118 s4 : ffc80400c000
 s5 : ffc80400c000 s6 : 00e7 s7 : 
 s8 : ffc80400bfff s9 : 00fff000 s10: ffc804008000
 s11: ffe7bfe0d100 t3 :  t4 : fff0
 t5 : 000f t6 : ffe7bfe22ac4
status: 00020120 badaddr:  cause: 
0003
[] __vmalloc_node_range+0x392/0x52e
[] copy_process+0x636/0x1196
[] kernel_clone+0x4a/0x2f4
[] user_mode_thread+0x7c/0x9a
[] rest_init+0x28/0xea
[] arch_post_acpi_subsys_init+0x0/0x18
[] start_kernel+0x72c/0x75c

In my mind, there's a kernel misconfiguration issue, but it's not the
same splat as I used to get when using FLATMEM in this configuration:
OF: fdt: Ignoring memory range 0x8000 - 0x8020
Machine model: Microchip PolarFire-SoC Icicle Kit
earlycon: ns16550a0 at MMIO32 0x2010 (options '115200n8')
printk: bootconsole [ns16550a0] enabled
printk: debug: skip boot console de-registration.
efi: UEFI not found.
Zone ranges:
  DMA32[mem 0x8020-0x]
  Normal   [mem 0x0001-0x00107fff]
Movable zone start for each node
Early memory node ranges
  node   0: [mem 0x8020-0xbfbf]
  node   0: [mem 0xbfc0-0xbfff]
  node   0: [mem 0x00104000-0x00107fff]
Initmem setup node 0 [mem 0x8020-0x00107fff]
Kernel panic - not syncing: Failed to allocate 1073741824 bytes for 
node 0 memory map
CPU: 0 PID: 0 Comm: swapper Not tainted 5.19.0-dirty #1
Hardware name: Microchip PolarFire-SoC Icicle Kit (DT)
Call Trace:
[] show_stack+0x30/0x3c
[] dump_stack_lvl+0x4a/0x66
[] dump_stack+0x18/0x20
[] panic+0x124/0x2c6
[] free_area_init_core+0x0/0x11e
[] free_area_init_node+0xc2/0xf6
[] free_area_init+0x222/0x260
[] misc_mem_init+0x62/0x9a
[] setup_arch+0xb0/0xea
[] start_kernel+0x88/0x4ee

Turning on SPARSEMEM fixes both problems, but I'm just not sure if there
is some underlying issue here that I don't know enough about the area to
understand.

Thanks,
Conor.

[0.00] Linux version 6.2.0-rc4-00538-g6e265f6be0c8 (conor@wendy) 
(riscv64-unknown-linux-gnu-gcc (g5964b5cd727)
 11.1.0, GNU ld (GNU Binutils) 2.37) #1 SMP PREEMPT @7
[0.00] OF: fdt: Ignoring memory range 0x8000 - 0x8020
[0.00] Machine model: Microchip PolarFire-SoC Icicle Kit
[0.00] earlycon: ns16550a0 at MMIO32 0x2010 (options 
'115200n8')
[0.00] printk: bootconsole [ns16550a0] enabled
[0.00

Re: [PATCH v2 0/4] PCI: Remove unnecessary includes

[Conor Dooley]

On Tue, Oct 25, 2022 at 01:51:43PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas 
> 
> Many host controller drivers #include  even though they
> don't need it.  Remove the unnecessary #includes.
> 
> v1: https://lore.kernel.org/all/20221019195452.37606-1-helg...@kernel.org/
> 
> Changes from v1 to v2:
>   - Include  explicitly in altera-msi and microchip,
> which don't need  itself, but relied on it to include
> 
>   - Include  explicitly in mvebu, which needs both it
> and 
> 
> Bjorn Helgaas (4):
>   PCI: altera-msi: Include  explicitly
>   PCI: microchip: Include  explicitly
>   PCI: mvebu: Include  explicitly
>   PCI: Remove unnecessary  includes

>  drivers/pci/controller/pcie-microchip-host.c | 2 +-

Hey Bjorn, actually did the build this time rather than visually
inspecting... For the microchip bits:
Reviewed-by: Conor Dooley 
Thanks!

Re: [PATCH] PCI: Remove unnecessary of_irq.h includes

[Conor Dooley]

On Thu, Oct 20, 2022 at 08:45:47AM -0500, Bjorn Helgaas wrote:
> [+cc Pali, heads-up for trivial addition of  to
> pci-mvebu.c]
> 
> On Thu, Oct 20, 2022 at 08:20:25AM +0100, Conor Dooley wrote:
> > On Thu, Oct 20, 2022 at 03:08:50PM +0800, kernel test robot wrote:
> > > Hi Bjorn,
> > > 
> > > I love your patch! Yet something to improve:
> > > 
> > > >> drivers/pci/controller/pcie-microchip-host.c:473:31: error: incomplete 
> > > >> definition of type 'struct irq_domain'
> > >struct mc_pcie *port = domain->host_data;
> > 
> > That's what I get for only visually inspecting the patch before Acking
> > it.. Un-ack I suppose.
> 
> No problem!
> 
> I think what happened is the pcie-microchip-host.c uses
> irq_domain_add_linear() so it needs , but it
> currently gets it via , which it doesn't otherwise
> need.
> 
> I added a preparatory patch to include  explicitly,
> but I haven't been able to cross-build either riscv or ia64 to verify
> this fix.  I'll wait a few days and post an updated series for the
> 0-day bot to test.

I saw you saying you couldn't find the config from LKP, FWIW a build
using riscv defconfig w/ CONFIG_PCIE_MICROCHIP_HOST=y fails for me
in the same way as lkp reports.
Otherwise, dump the patch in response to this and I'll give it a shot
later if you like?

HTH,
Conor.

> 
> Same situation for pcie-altera-msi.c.
> 
> pci-mvebu.c also relies on getting  via
> , but it actually depends on of_irq.h, so I'll just
> add an irqdomain.h include there.
> 
> Bjorn
> 

Re: [PATCH] PCI: Remove unnecessary of_irq.h includes

[Conor Dooley]

On Wed, Oct 19, 2022 at 02:54:51PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas 
> 
> Many host controller drivers #include  even though they
> don't need it.  Remove the unnecessary #includes.
> 
> Signed-off-by: Bjorn Helgaas 

>  drivers/pci/controller/pcie-microchip-host.c | 1 -

LGTM...
Acked-by: Conor Dooley 

Re: [PATCH v7 00/25] Rust support

[Conor Dooley]

Hey,

Maybe I am just missing something blatantly obvious here, but trying
to build rust support in -next fails for me. I am using ClangBuiltLinux
clang version 15.0.0 5b0788fef86ed7008a11f6ee19b9d86d42b6fcfa and LLD
15.0.0. Is it just expected that building -next with rust support is
not a good idea?
My defconfig is the default RISC-V one plus:
CONFIG_RUST=y
CONFIG_SAMPLES=y
CONFIG_SAMPLES_RUST=y
CONFIG_SAMPLE_RUST_MINIMAL=y

Thanks,
Conor.

Fail log:
  UPD rust/target.json
  BINDGEN rust/bindings_generated.rs
  BINDGEN rust/bindings_helpers_generated.rs
  RUSTC L rust/core.o
  EXPORTS rust/exports_core_generated.h
  RUSTC P rust/libmacros.so
  RUSTC L rust/compiler_builtins.o
  RUSTC L rust/alloc.o
  RUSTC L rust/build_error.o
  EXPORTS rust/exports_alloc_generated.h
  RUSTC L rust/kernel.o
error[E0428]: the name `maple_enode` is defined multiple times
 --> linux/rust/bindings_generated.rs:18009:1
  |
18006 | pub struct maple_enode {
  | -- previous definition of the type `maple_enode` 
here
...
18009 | pub type maple_enode = *mut maple_enode;
  |  `maple_enode` redefined here
  |
  = note: `maple_enode` must be defined only once in the type namespace of 
this module

error[E0428]: the name `maple_pnode` is defined multiple times
 --> linux/rust/bindings_generated.rs:18015:1
  |
18012 | pub struct maple_pnode {
  | -- previous definition of the type `maple_pnode` 
here
...
18015 | pub type maple_pnode = *mut maple_pnode;
  |  `maple_pnode` redefined here
  |
  = note: `maple_pnode` must be defined only once in the type namespace of 
this module

error[E0391]: cycle detected when expanding type alias 
`bindings::bindings_raw::maple_pnode`
 --> linux/rust/bindings_generated.rs:18015:29
  |
18015 | pub type maple_pnode = *mut maple_pnode;
  | ^^^
  |
  = note: ...which immediately requires expanding type alias 
`bindings::bindings_raw::maple_pnode` again
  = note: type aliases cannot be recursive
  = help: consider using a struct, enum, or union instead to break the cycle
  = help: see 
 for more 
information
note: cycle used when computing type of 
`bindings::bindings_raw::maple_range_64::parent`
 --> linux/rust/bindings_generated.rs:18058:22
  |
18058 | pub parent: *mut maple_pnode,
  |  ^^^

error[E0391]: cycle detected when expanding type alias 
`bindings::bindings_raw::maple_enode`
 --> linux/rust/bindings_generated.rs:18009:29
  |
18009 | pub type maple_enode = *mut maple_enode;
  | ^^^
  |
  = note: ...which immediately requires expanding type alias 
`bindings::bindings_raw::maple_enode` again
  = note: type aliases cannot be recursive
  = help: consider using a struct, enum, or union instead to break the cycle
  = help: see 
 for more 
information
note: cycle used when computing type of 
`bindings::bindings_raw::maple_topiary::next`
 --> linux/rust/bindings_generated.rs:18340:20
  |
18340 | pub next: *mut maple_enode,
  |^^^

error[E0117]: only traits defined in the current crate can be implemented for 
arbitrary types
 --> linux/rust/bindings_generated.rs:18005:10
  |
18005 | #[derive(Copy, Clone)]
  |  
  |  |
  |  impl doesn't use only types from inside the current crate
  |  `*mut [type error]` is not defined in the current crate
  |
  = note: define and implement a trait or new type instead
  = note: this error originates in the derive macro `Copy` (in Nightly 
builds, run with -Z macro-backtrace for more info)

error[E0117]: only traits defined in the current crate can be implemented for 
arbitrary types
 --> linux/rust/bindings_generated.rs:18011:10
  |
18011 | #[derive(Copy, Clone)]
  |  
  |  |
  |  impl doesn't use only types from inside the current crate
  |  `*mut [type error]` is not defined in the current crate
  |
  = note: define and implement a trait or new type instead
  = note: this error originates in the derive macro `Copy` (in Nightly 
builds, run with -Z macro-backtrace for more info)

error[E0117]: only traits defined in the current crate can be implemented for 
arbitrary types
 --> linux/rust/bindings_generated.rs:18005:16
  |
18005 | #[derive(Copy, Clone)]
  |^
  ||
  |impl doesn't use only types from inside the current crate
  |`*mut [type error]` is not defined in the current crate
  |
  = note: define and implement a